Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix time window effect on birth event selection range #25

Merged
merged 2 commits into from
Apr 17, 2022
Merged

Fix time window effect on birth event selection range #25

merged 2 commits into from
Apr 17, 2022

Conversation

hugy718
Copy link
Collaborator

@hugy718 hugy718 commented Apr 5, 2022

Fix #24

@hugy718 hugy718 requested a review from KimballCai April 5, 2022 13:56
@hugy718 hugy718 linked an issue Apr 5, 2022 that may be closed by this pull request
Missing entrires in results of query1-0.json #24
Closed
@hugy718
Copy link
Collaborator Author

hugy718 commented Apr 5, 2022

@KimballCai found another potential issue. In the same function. The birthOffset assignment on L485 does not match the comment. It is returned via cohort. Not sure if this is working as expected.
I tried switching the condition to (birthoffset > offset) It only affected the query result in one result tuple ExtendedResultTuple(cohort=((1960, 1970]), age=0, measure=1594.0, min=0.0, max=85.0, sum=11888.0, num=381.0) by increasing the measure by 1. Others are the same.

KimballCai
KimballCai requested changes Apr 6, 2022
View reviewed changes
Copy link
Contributor

@KimballCai KimballCai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

need to add the logic to filter users by the time winsow setting.

cool-core/src/main/java/com/nus/cool/core/cohort/ExtendedCohortSelection.java
@@ -423,7 +423,7 @@ private int getUserBirthTime(int start, int end) {
// with time window
int startDay = firstDay;
int wlen = window.getLength();
int endDay = maxDate - (wlen - 1); // at least one time window
int endDay = TimeUtils.getDate(timeVector.get(end - 1)); // it is open for the time window to overflow to date not seen in dataset. We don't search beyong the last date user record appears
int windowOffset;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should filter users who do not have records for at least wlen days.

@KimballCai
Copy link
Contributor

Fix the issue #24 with the problem about the TimeWindow

@KimballCai KimballCai merged commit 54f0f8e into main Apr 17, 2022
@KimballCai KimballCai deleted the 24-missing-entrires-in-results-of-query-json branch April 17, 2022 18:08
@KimballCai KimballCai mentioned this pull request Apr 18, 2022
Closed
@KimballCai KimballCai mentioned this pull request Apr 29, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@KimballCai KimballCai KimballCai requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Missing entrires in results of query1-0.json
2 participants
@hugy718 @KimballCai