-
Notifications
You must be signed in to change notification settings - Fork 171
Temporal annual distribution functionality and temporal FeatureExtraction capability enablement #2388
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
base: master
Are you sure you want to change the base?
Conversation
It looks like only the 'annual' flag is currently transferred from the client which leads to storing NULL in the database
…e covariates comparison feature
[ATL-10] Added an option to exclude creating comparative items for the covariates comparison feature
…ke it was done for the annual Feature Analysis flag
…l distribution functionality should be available
@@ -26,4 +26,42 @@ CREATE TABLE @results_schema.cc_results | |||
aggregate_id INTEGER, | |||
aggregate_name VARCHAR(1000), | |||
missing_means_zero INTEGER | |||
); | |||
|
|||
IF OBJECT_ID('@results_schema.cc_temporal_results') IS NULL |
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.
Similar notation as for the table above should be used
IF OBJECT_ID('@results_schema.table', 'U') IS NULL
'DemographicsPriorObservationTime', | ||
'DemographicsPostObservationTime', | ||
'DemographicsTimeInCohort', | ||
'DemographicsIndexYearMonth' |
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.
Is the list final?
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.
…e covariates comparison feature
[ATL-10] Added an option to exclude creating comparative items for the covariates comparison feature
…ke it was done for the annual Feature Analysis flag
… with missing design
Fixed NullPointerException during saving of a cohort characterization with missing design
…nual-distribution
…ts schema DDL Supplementing Feature Analyses names from FeatureExtraction for the temporal feature
Chris, please pull the code @chrisknoll |
Error when generating:
This came from a copy of an existing CC, where I switched on the new options in the design. Attached is sample CC example: CC_TestCase.txt |
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.
Will approve once errors are resolved.
Hi @chrisknoll , @anthonysena . I was able to reproduce the problem, a small fix on the SkeletonCohortCharacterization was enough for the test CC you've provided to get generated locally (Report opens, the temporal distributions are in place). I assume we`ll need to release the skeleton lib once again.. Fix PR: SkeletonCohortCharacterization PR 47 , Thank you. |
Yes, please release SkeletonCC, and as part of this release if we could add this failure as a test case for the package. |
Hi @chrisknoll , @anthonysena , thank you for releasing SkeletonCohortCharacterization v2.0.1. Looks like the CI for this PR now succeeds and there are no merge conflicts so far. Please kindly merge if you have no other remarks. Thank you. |
While the functionality seems to work, my concern is on execution time. Here's a screen-shot of 2 executions, the 5 minute one is without the new options, the 13 hour one is with the options enabled. This was running on a large sample CDM on my local box: So I'm concerned about releasing this feature and leading to 100-fold increase in execution time. Maybe we can spend some time at the meeting evaluating the queries and understanding if there's a performance optimization to be made. |
@oleg-odysseus : per our conversation on WG call, you'll investigate the comparison flag that is leading to an empty STDDiff column in the UI. For context: it seems that the new atlas branch shows the correct STDDiff values when the WebAPI branch is 'master'...only switching to the WebAPI temporal prevalence branch leads to the error in the UI where the StdDiff column is empty. |
…s with temporal distributions for a scenario with 2 cohorts.
@chrisknoll , I've reproduced the problem locally and decided to roll back the change that I've introduced some time ago that led to this behavior (that change forced disabling the comparative results). Now as per my local checks, the reports behave as usual for the analysis with multiple cohorts. If the analysis was generated with temporal distribution then the links to observe those distributions would be visible for 1, 3 or more cohorts. For the comparative mode (2 cohorts) the link is not enabled, but if a user needs to observe temporal/annual distribution for the cohort then he/she could exit the comparative mode by selecting, for example, only that particular cohort and observing temporal distributions for it by clicking the link. |
Addressing #2331 (temporal annual distribution requirement)
Cohort Characterization Controller has been extended with a new endpoint
GET /generation/{generationId}/temporalresult
Temporal annual distribution and temporal Cohort Characterization analysis execution results are selected from newly introduced tables in the result schema - cc_temporal_results and cc_temporal_annual_results
Each Feature Analysis has been extended with two attributes indicating their temporal and temporal annual distribution properties depending on their actual support by FeatureExtraction