-
Notifications
You must be signed in to change notification settings - Fork 169
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
Cohort demographics results are available for visualization if explicitly requested while executing #2402
base: master
Are you sure you want to change the base?
Conversation
Use cohort characterization function to generate cohort with demographic generate cohort definition with demoraphic, get the result and store at cc_result Add function to get demographic report from cohort definition # Conflicts: # src/main/java/org/ohdsi/webapi/Constants.java # src/main/java/org/ohdsi/webapi/cohortdefinition/CohortGenerationRequest.java # src/main/java/org/ohdsi/webapi/cohortdefinition/CohortGenerationRequestBuilder.java # src/main/java/org/ohdsi/webapi/cohortdefinition/GenerateCohortTasklet.java # src/main/java/org/ohdsi/webapi/service/CohortDefinitionService.java # src/main/java/org/ohdsi/webapi/service/CohortGenerationService.java # src/main/java/org/ohdsi/webapi/shiny/CohortCountsShinyPackagingService.java
# Conflicts: # src/main/java/org/ohdsi/webapi/cohortdefinition/CohortGenerationUtils.java
…he Data Source one Adding a separate migration script instead of changing the old ones (Oracle and SQL Server are no longer supported application dialects) # Conflicts: # src/main/java/org/ohdsi/webapi/service/CohortGenerationService.java
…was inconsistent # Conflicts: # src/main/java/org/ohdsi/webapi/service/CohortGenerationService.java
// Get FE Analysis Demographic (Gender, Age, Race,) | ||
Set<FeAnalysisEntity> feAnalysis = feAnalysisRepository.findByListIds(Arrays.asList(70, 72, 74, 77)); | ||
|
||
// Set<CcFeAnalysisEntity> ccFeAnalysis = feAnalysis.stream().map(a -> { |
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.
It relates to https://github.com/OHDSI/WebAPI/pull/2388/files when it is merged to 'master'
return prepareQueriesDefault(jobParams, jdbcTemplate); | ||
} | ||
|
||
private String[] prepareQueriesDemographic(ChunkContext chunkContext, CancelableJdbcTemplate jdbcTemplate) { |
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.
This logic has been taken from Cohort Characterization
try { | ||
sql = BigQuerySparkTranslate.sparkHandleInsert(sql, source.getSourceConnection()); | ||
} catch (SQLException e) { | ||
e.printStackTrace(); |
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.
A log entry should be created instead both in the source code from where it was copied and here
@@ -84,6 +84,21 @@ public CohortGenerationInfo(CohortDefinition definition, Integer sourceId) | |||
@ManyToOne(fetch = FetchType.LAZY) | |||
@JoinColumn(name = "created_by_id") | |||
private UserEntity createdBy; | |||
|
|||
@Column(name = "cc_generate_id") | |||
private Long ccGenerateId; |
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.
This identifier is necessary to correlate a Cohort execution with associated demographics Cohort Characterization
@@ -42,5 +44,10 @@ public static class InclusionRuleStatistic | |||
public Summary summary; | |||
public List<InclusionRuleStatistic> inclusionRuleStats; | |||
public String treemapData; | |||
public List<Report> demographicsStats; |
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.
There should be found a way not to extend the existing InclusionRuleReport class as they don't relate to each other
@@ -819,31 +1085,45 @@ public Response exportConceptSets(@PathParam("id") final int id) { | |||
public InclusionRuleReport getInclusionRuleReport( | |||
@PathParam("id") final int id, | |||
@PathParam("sourceKey") final String sourceKey, | |||
@DefaultValue("0") @QueryParam("mode") int modeId) { | |||
@DefaultValue("0") @QueryParam("mode") int modeId, @QueryParam("ccGenerateId") String ccGenerateId) { |
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.
Formatting is broken
@@ -0,0 +1,2 @@ | |||
ALTER TABLE ${ohdsiSchema}.cohort_generation_info ADD is_choose_demographic BOOLEAN NOT NULL DEFAULT FALSE; |
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.
Awkward name. Simply 'is_demographic should suffice'
@@ -0,0 +1,2 @@ | |||
ALTER TABLE ${ohdsiSchema}.cohort_generation_info ADD is_choose_demographic BOOLEAN NOT NULL DEFAULT FALSE; | |||
ALTER TABLE ${ohdsiSchema}.cohort_generation_info ADD cc_generate_id INTEGER 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.
what is the need for a cohort characterization generation ID? we don't want to confuse cohort characterization with this demographics summary....The issue will be that as we clean up jobs we'll want to find jobs related to cohort characterization (actual cohort characterization entities) and if this cc_generate_id doesn't correspond over to characterization design, it will introduce confusion.
Perhaps I need to understand better how this function interacts with cohort characterization?
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.
Also, we should label this migration as 2.15.
Addressing #2347