-
Notifications
You must be signed in to change notification settings - Fork 17
Enabling calculations in between two related Measurements by adding a new criteria attribute #210
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?
Changes from all commits
b95f63d
75a2906
77a2338
d42ba73
22975c8
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,16 @@ | ||
| package org.ohdsi.circe.cohortdefinition; | ||
|
|
||
| import com.fasterxml.jackson.annotation.JsonProperty; | ||
|
|
||
| public class MeasurementOperand { | ||
| @JsonProperty("Measurement") | ||
| public Measurement measurement; | ||
| @JsonProperty("Operator") | ||
| public String operator; | ||
| @JsonProperty("Limit") | ||
| public String limit = "First"; | ||
| @JsonProperty("ValueAsNumber") | ||
| public NumericRange valueAsNumber; | ||
| @JsonProperty("SameVisit") | ||
| public Boolean sameVisit = true; | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -30,25 +30,29 @@ public String getCriteriaSql(T criteria, BuilderOptions options) { | |
| query = embedJoinClauses(query, joinClauses); | ||
| query = embedWhereClauses(query, whereClauses); | ||
|
|
||
| query = embedAdditionalColumns(query, criteria, options); | ||
|
|
||
| return query; | ||
| } | ||
|
|
||
| protected String embedAdditionalColumns(String query, T criteria, BuilderOptions options) { | ||
| if (options != null) { | ||
| List<CriteriaColumn> filteredColumns = options.additionalColumns.stream() | ||
| .filter((column) -> !this.getDefaultColumns().contains(column)) | ||
| .collect(Collectors.toList()); | ||
| .filter((column) -> !this.getDefaultColumns().contains(column)) | ||
| .collect(Collectors.toList()); | ||
| if (filteredColumns.size() > 0) { | ||
| query = StringUtils.replace(query, "@additionalColumns", ", " + this.getAdditionalColumns(filteredColumns)); | ||
| return StringUtils.replace(query, "@additionalColumns", ", " + this.getAdditionalColumns(criteria, filteredColumns)); | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I believe changing the 3 query assignments into 3 individual returns doesn't improve anything with the code, and makes it harder to debug when you just want to know what the query result that will be returned. |
||
| } else { | ||
| query = StringUtils.replace(query, "@additionalColumns", ""); | ||
| return StringUtils.replace(query, "@additionalColumns", ""); | ||
| } | ||
| } else { | ||
| query = StringUtils.replace(query, "@additionalColumns", ""); | ||
| return StringUtils.replace(query, "@additionalColumns", ""); | ||
| } | ||
|
|
||
| return query; | ||
| } | ||
|
|
||
| protected abstract String getTableColumnForCriteriaColumn(CriteriaColumn column); | ||
|
|
||
| protected String getAdditionalColumns(List<CriteriaColumn> columns) { | ||
| protected String getAdditionalColumns(T criteria, List<CriteriaColumn> columns) { | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why was the criteria parameter added to this function when it's not even used in it? |
||
| String cols = String.join(", ", columns.stream() | ||
| .map((column) -> { | ||
| return String.format("%s as %s", getTableColumnForCriteriaColumn(column), column.columnName()); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4,11 +4,18 @@ | |
| import org.ohdsi.circe.cohortdefinition.Measurement; | ||
| import org.ohdsi.circe.helper.ResourceHelper; | ||
|
|
||
| import java.text.MessageFormat; | ||
| import java.util.ArrayList; | ||
| import java.util.Arrays; | ||
| import java.util.Collections; | ||
| import java.util.HashSet; | ||
| import java.util.List; | ||
| import java.util.Objects; | ||
| import java.util.Optional; | ||
| import java.util.Set; | ||
| import java.util.stream.Collectors; | ||
| import java.util.stream.Stream; | ||
|
|
||
| import org.ohdsi.circe.cohortdefinition.DateAdjustment; | ||
|
|
||
| import static org.ohdsi.circe.cohortdefinition.builders.BuilderUtils.buildDateRangeClause; | ||
|
|
@@ -25,7 +32,7 @@ public class MeasurementSqlBuilder<T extends Measurement> extends CriteriaSqlBui | |
|
|
||
| // default select columns are the columns that will always be returned from the subquery, but are added to based on the specific criteria | ||
| private final List<String> DEFAULT_SELECT_COLUMNS = new ArrayList<>(Arrays.asList("m.person_id", "m.measurement_id", "m.measurement_concept_id", "m.visit_occurrence_id", | ||
| "m.value_as_number", "m.range_high", "m.range_low")); | ||
| "m.value_as_number", "m.range_high", "m.range_low")); | ||
|
|
||
| @Override | ||
| protected Set<CriteriaColumn> getDefaultColumns() { | ||
|
|
@@ -38,6 +45,34 @@ protected String getQueryTemplate() { | |
| return MEASUREMENT_TEMPLATE; | ||
| } | ||
|
|
||
| /* | ||
|
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. To be deleted |
||
| @Override | ||
| protected String embedAdditionalColumns(String query, T criteria, BuilderOptions options) { | ||
| if (criteria.measurementOperand != null) { | ||
| String additionalColumns = getAdditionalColumns(criteria, Optional.ofNullable(options).map(o -> o.additionalColumns).orElse(new ArrayList<>())); | ||
| return StringUtils.replace(query, "@additionalColumns", additionalColumns); | ||
| } else { | ||
| return super.embedAdditionalColumns(query, criteria, options); | ||
| } | ||
| } | ||
| */ | ||
|
|
||
| /* | ||
| @Override | ||
| protected String getAdditionalColumns(T criteria, List<CriteriaColumn> columns) { | ||
| return Stream.concat( | ||
| Stream.of(super.getAdditionalColumns(criteria, columns)), | ||
| Optional.ofNullable(criteria.measurementOperand).map(measurementOperand -> | ||
| Stream.of( | ||
| MessageFormat.format("row_number() over (PARTITION BY C.person_id ORDER BY SC.start_date, SC.event_id {0}) as ordinal", | ||
| Optional.ofNullable(measurementOperand.limit).map(limit -> limit.equals("Last") ? "DESC" : "").orElse("")) | ||
| ) | ||
| ).orElse(Stream.empty()) | ||
| ) | ||
| .collect(Collectors.joining(",")); | ||
| } | ||
| */ | ||
|
|
||
| @Override | ||
| protected String getTableColumnForCriteriaColumn(CriteriaColumn column) { | ||
| switch (column) { | ||
|
|
@@ -60,10 +95,10 @@ protected String getTableColumnForCriteriaColumn(CriteriaColumn column) { | |
| protected String embedCodesetClause(String query, T criteria) { | ||
|
|
||
| return StringUtils.replace(query, "@codesetClause", | ||
| getCodesetJoinExpression(criteria.codesetId, | ||
| "m.measurement_concept_id", | ||
| criteria.measurementSourceConcept, | ||
| "m.measurement_source_concept_id") | ||
| getCodesetJoinExpression(criteria.codesetId, | ||
| "m.measurement_concept_id", | ||
| criteria.measurementSourceConcept, | ||
| "m.measurement_source_concept_id") | ||
| ); | ||
| } | ||
|
|
||
|
|
@@ -115,11 +150,12 @@ protected List<String> resolveSelectClauses(T criteria) { | |
| // dateAdjustment or default start/end dates | ||
| if (criteria.dateAdjustment != null) { | ||
| selectCols.add(BuilderUtils.getDateAdjustmentExpression(criteria.dateAdjustment, | ||
| criteria.dateAdjustment.startWith == DateAdjustment.DateType.START_DATE ? "m.measurement_date" : "DATEADD(day,1,m.measurement_date)", | ||
| criteria.dateAdjustment.endWith == DateAdjustment.DateType.START_DATE ? "m.measurement_date" : "DATEADD(day,1,m.measurement_date)")); | ||
| criteria.dateAdjustment.startWith == DateAdjustment.DateType.START_DATE ? "m.measurement_date" : "DATEADD(day,1,m.measurement_date)", | ||
| criteria.dateAdjustment.endWith == DateAdjustment.DateType.START_DATE ? "m.measurement_date" : "DATEADD(day,1,m.measurement_date)")); | ||
| } else { | ||
| selectCols.add("m.measurement_date as start_date, DATEADD(day,1,m.measurement_date) as end_date"); | ||
| } | ||
|
|
||
| return selectCols; | ||
| } | ||
|
|
||
|
|
@@ -138,6 +174,15 @@ protected List<String> resolveJoinClauses(T criteria) { | |
| if (criteria.providerSpecialty != null && criteria.providerSpecialty.length > 0) { | ||
| joinClauses.add("LEFT JOIN @cdm_database_schema.PROVIDER PR on C.provider_id = PR.provider_id"); | ||
| } | ||
| if (criteria.measurementOperand != null && criteria.measurementOperand.measurement != null) { | ||
| BuilderOptions options = new BuilderOptions(); | ||
| options.additionalColumns = Collections.singletonList(CriteriaColumn.VALUE_AS_NUMBER); | ||
| String subquery = getCriteriaSql((T) criteria.measurementOperand.measurement, options); | ||
| joinClauses.add(MessageFormat.format("JOIN (SELECT E.person_id, E.event_id, E.visit_occurrence_id, E.value_as_number, row_number() over (PARTITION BY E.person_id ORDER BY E.start_date, E.event_id {1}) as ordinal FROM (\n{0}\n) E) SC on C.person_id = SC.person_id", | ||
| subquery, | ||
| Optional.ofNullable(criteria.measurementOperand.limit).map(limit -> limit.equals("Last") ? "DESC" : "").orElse("") | ||
| )); | ||
| } | ||
|
Comment on lines
+177
to
+185
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should there be time-bounding to looking for these measurements (either prior to or post the 'index event'). I believe you would want to specify this somehow, and maybe the extent of the prior/post time to select records from. I understand 'same visit' addresses some of this, but I don't think you can depend on Measurement domains to come from the same visit, especially if the idea is to compare 2 measurements that could be spread across different visits. |
||
|
|
||
| return joinClauses; | ||
| } | ||
|
|
@@ -226,6 +271,18 @@ protected List<String> resolveWhereClauses(T criteria) { | |
| whereClauses.add(String.format("V.visit_concept_id in (%s)", StringUtils.join(getConceptIdsFromConcepts(criteria.visitType), ","))); | ||
| } | ||
|
|
||
| //Calculation | ||
| if (criteria.measurementOperand != null) { | ||
| whereClauses.add(buildNumericRangeClause( | ||
| MessageFormat.format("(C.value_as_number {0} SC.value_as_number)", criteria.measurementOperand.operator), | ||
| criteria.measurementOperand.valueAsNumber, ".4f") | ||
|
Comment on lines
+276
to
+278
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm worried that we embed the operator (ie the sql operator) as the string in the payload. I feel like the operators should be a distinct list of supported operators, and based on the user selection we translate it into the sql operation. And, it's not quite clear how this is used. The template is: So can the operator be Or is the operator supposed to be an arithmetic operation (+, - , / *) Unfortunately, I can't find documentation on this feature about how it's supposed to be used and what functions it supports. |
||
| ); | ||
| if (Optional.ofNullable(criteria.measurementOperand.sameVisit).orElse(false)) { | ||
| whereClauses.add("SC.visit_occurrence_id = C.visit_occurrence_id"); | ||
| } | ||
| whereClauses.add("SC.ordinal = 1"); | ||
| } | ||
|
|
||
| return whereClauses; | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -151,7 +151,9 @@ c.visitType??><#local temp>a visit occurrence that is: <@inputTypes.ConceptList | |
| measurement<#if isPlural && !(c.first!false)>s</#if> of ${utils.codesetName(c.codesetId!"", "any measurement")}<#if | ||
| c.measurementSourceConcept??> (including ${utils.codesetName(c.measurementSourceConcept, "any measurement")} source concepts)</#if><#if | ||
| c.first!false> for the first time in the person's history</#if><#if attrs?size gt 0>, ${attrs?join("; ")}</#if><#if | ||
| c.CorrelatedCriteria??>; <@Group group=c.CorrelatedCriteria level=level indexLabel=utils.codesetName(c.codesetId!"", "any measurement") /><#else>.</#if></#macro> | ||
| c.CorrelatedCriteria??>; <@Group group=c.CorrelatedCriteria level=level indexLabel=utils.codesetName(c.codesetId!"", "any measurement") /><#else>.</#if> | ||
| <#if c.measurementOperand??>numeric value calculated as <@MeasurementOperand c=c.measurementOperand level=level+1 /></#if><#nested /> | ||
|
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We may need to enhance it so that the sentence is better readable |
||
| </#macro> | ||
|
|
||
| <#macro Observation c level isPlural=true countCriteria={} indexLabel="cohort entry"><#local attrs = []><#local attrs = []><#if countCriteria?has_content> | ||
| <#local temp><@WindowCriteria countCriteria=countCriteria indexLabel=indexLabel/></#local><#if temp?has_content><#local attrs+=[temp]></#if></#if> | ||
|
|
@@ -308,3 +310,14 @@ temp><@EventDateCriteria c.occurrenceStartDate!{} c.occurrenceEndDate!{} /></#lo | |
| c.race??><#local temp>race is: <@inputTypes.ConceptList list=c.race/></#local><#local attrs+=[temp]></#if><#if | ||
| c.ethnicity??><#local temp>ethnicity is: <@inputTypes.ConceptList list=c.ethnicity/></#local><#local attrs+=[temp]></#if><#if | ||
| attrs?size gt 0>with the following event criteria: ${attrs?join("; ")}.<#else>any event (no demographic criteria specified).</#if></#macro> | ||
|
|
||
| <#-- Calculation Criteria --> | ||
| <#macro MeasurementOperand c level>primary measurement ${c.operator} <#local temp><@MeasurementLimit limit = c.limit /></#local> <#local temp><@Measurement c=c.measurement level=level+1> <#if c.sameVisit>in the same visit</#if> <#if c.valueAsNumber??>which are <@inputTypes.NumericRange range=c.valueAsNumber /></#if> </@Measurement></#local> ${temp}</#macro> | ||
|
|
||
| <#-- Limits --> | ||
| <#assign measurementLimitOptions = [ | ||
| {"id": "First", "name": "earliest event"}, | ||
| {"id": "Last", "name": "latest event"} | ||
| ]/> | ||
|
|
||
| <#macro MeasurementLimit limit>${utils.optionName(measurementLimitOptions, limit)}</#macro> | ||
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.
I'm not sure this needs to be true by default. Most of the time, we default boolean values to false.