-
Notifications
You must be signed in to change notification settings - Fork 61
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.
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
Temporal annual distribution functionality #271
base: develop
Are you sure you want to change the base?
Temporal annual distribution functionality #271
Conversation
Referencing SqlRender 1.18.1 Version 3.7.0-SNAPSHOT as 3.6.0 has been released even though there were no changes to pom.xml Merge remote-tracking branch 'remotes/origin/ATL-10' into develop # Conflicts: # inst/java/featureExtraction-3.3.0-SNAPSHOT.jar # java/org/ohdsi/featureExtraction/FeatureExtraction.java
@anthonysena Anthony, I can't assign reviewers, please do so |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@alex-odysseus - I've provided some feedback on this PR for your review. I have not reviewed the other associated PRs so I'm unsure of the impact this proposed change would have on this feature. That said, here is my feedback:
- We need to make sure any enhancements to FeatureExtraction are done so that users from the R side can access this functionality. As it currently stands, I don't see a way that an R user could invoke this functionality.
- I think we can simplify the way in which this feature is invoked by simply passing in an option to perform the annual distribution as noted in the comments.
- All R command checks and unit tests must pass for this to be considered in a state for merging into the
develop
branch.
Thanks!
@@ -11,15 +11,27 @@ SELECT | |||
row_id, | |||
1 AS covariate_value | |||
} | |||
{@temporal_annual} ? {, event_year} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The temporal_annual
appears to be the way in which a user would indicate to FeatureExtraction to compute the annual distribution yet this parameter is never exposed to the user best I can tell. In reviewing the code, the approach appears to be: specify the analysis IDs in the PrespecTemporalAnnualAnalysis.csv
and then the Java layer will then inject the temporal_annual
parameter value.
@alex-odysseus thanks for reviewing this with me on Atlas/WebAPI WG call. Here are some specific tasks from our discussion:
|
Supporting OHDSI/WebAPI#2331 to enable temporal annual distribution extending the existing temporal functionality
Temporal annual distribution will be effective for Feature Analyses specified in newly added PrespecTemporalAnnualAnalysis.csv
The idea of the functionality is to extract an underlying year for a Covariate of interest so that it can be easily grouped by in Cohort Characterization result visualization in WebAPI / ATLAS
Minor refactoring using Stream and StreamSupport classes
Standard Java formatting applied in a few places (hopefully acceptable)
Referencing SqlRender 1.18.1 in pom.xml
Targeting feature for 3.7.0 (for an unknown reason there is still 3.5.1-SNAPSHOT in 'develop' pom.xml even though 3.6.0 has been officially released