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.

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

Support for datetime calculations in cohort definitions #200

Open
wants to merge 27 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
27 commits
Select commit Hold shift + click to select a range
1fa1d69
Support for datetime calculations in cohort definitions
jcnamendiOdysseus Dec 18, 2023
b9b5820
update
jcnamendiOdysseus Dec 29, 2023
6fd8ecd
update/412023
jcnamendiOdysseus Jan 4, 2024
07a7160
add sql test
jcnamendiOdysseus Jan 8, 2024
eaf44d5
fix
jcnamendiOdysseus Jan 9, 2024
6dc3918
Merge remote-tracking branch 'origin/is/2886' into issues-2886
jcnamendiOdysseus Jan 9, 2024
603c800
update test
jcnamendiOdysseus Jan 9, 2024
fe4f7e5
fix indent on editor
jcnamendiOdysseus Jan 9, 2024
75a2906
Merge pull request #6 from OHDSI/master
alex-odysseus Jan 15, 2024
3210fae
Merge remote-tracking branch 'remotes/origin/master' into issues-2886
alex-odysseus Jan 15, 2024
e7f90a6
remove jar
jcnamendiOdysseus Jan 15, 2024
0036dc0
Merge remote-tracking branch 'origin/issues-2886' into issues-2886
jcnamendiOdysseus Jan 15, 2024
4d0cb33
Support for datetime calculations in cohort definitions
jcnamendiOdysseus Dec 18, 2023
55b2eef
update
jcnamendiOdysseus Dec 29, 2023
806c85b
update/412023
jcnamendiOdysseus Jan 4, 2024
8a4eb23
add sql test
jcnamendiOdysseus Jan 8, 2024
f819d5e
fix
jcnamendiOdysseus Jan 9, 2024
5e03963
update test
jcnamendiOdysseus Jan 9, 2024
de9527c
fix indent on editor
jcnamendiOdysseus Jan 9, 2024
ca6bbfd
remove jar
jcnamendiOdysseus Jan 15, 2024
cebb8c5
Revert "fix"
jcnamendiOdysseus Jan 16, 2024
2ada34d
Merge remote-tracking branch 'origin/issues-2886' into issues-2886
jcnamendiOdysseus Jan 17, 2024
6fb7fd2
add tests
jcnamendiOdysseus Jan 17, 2024
da13f6e
Adding IntervalUnit to Criteria to specify datetime table columns whi…
alex-odysseus Jan 23, 2024
690b273
Replace wildcard import with specific imports
jcnamendiOdysseus Jan 23, 2024
2f97201
Enhance time unit handling in compareTo() method
jcnamendiOdysseus Jan 25, 2024
4209ca0
Introduce time interval testing functionality and enhance the impleme…
jcnamendiOdysseus Jan 25, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 23 additions & 12 deletions src/main/java/org/ohdsi/circe/check/checkers/Comparisons.java
Original file line number Diff line number Diff line change
Expand Up @@ -20,9 +20,7 @@

import java.time.LocalDate;
import java.time.format.DateTimeParseException;
import java.util.Arrays;
import java.util.List;
import java.util.Objects;
import java.util.*;
import java.util.function.Predicate;
import java.util.stream.Collectors;
import org.apache.commons.lang3.builder.EqualsBuilder;
Expand All @@ -31,6 +29,12 @@

public class Comparisons {

private static final Map<String, Integer> TIME_UNIT_CONVERSION = new HashMap<>();
static {
TIME_UNIT_CONVERSION.put(IntervalUnit.HOUR.getName(), 60 * 60);
TIME_UNIT_CONVERSION.put(IntervalUnit.MINUTE.getName(), 60);
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

I wouldn't do it this way, it makes it seem like it's dynamic (ie: can change at runtime). Can't this function of conversion be associated to the enum itself?

public static Boolean startIsGreaterThanEnd(NumericRange r) {

return Objects.nonNull(r.value) && Objects.nonNull(r.extent) && r.value.intValue() > r.extent.intValue();
Expand Down Expand Up @@ -101,19 +105,26 @@ public static Predicate<Concept> compare(Concept source) {
.append(concept.vocabularyId, source.vocabularyId)
.build();
}

public static int compareTo(ObservationFilter filter, Window window) {

int range1 = filter.postDays + filter.priorDays;
int range2Start = 0, range2End = 0;
if (Objects.nonNull(window.start) && Objects.nonNull(window.start.days)) {
range2Start = window.start.coeff * window.start.days;
}
if (Objects.nonNull(window.end) && Objects.nonNull(window.end.days)) {
range2End = window.end.coeff * window.end.days;
int range1 , range2Start , range2End;
if(Objects.nonNull(window.start) && Objects.nonNull(window.start.days)){
range1 = filter.postDays + filter.priorDays;
range2Start = window.start.coeff * window.start.days;
range2End = Objects.nonNull(window.end) && Objects.nonNull(window.end.days) ? window.end.coeff * window.end.days : 0;
}else {
range1 = (filter.postDays + filter.priorDays) * 24 * 60 * 60;
range2Start = getTimeInSeconds(window.start);
range2End = getTimeInSeconds(window.end);
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is where the 4sp indent is glaring, so before you have to go back and change all of it back to 2sp, i'd adjust your formatting.

}
return range1 - (range2End - range2Start);
}
private static int getTimeInSeconds(Window.Endpoint endpoint) {
return Optional.ofNullable(endpoint)
.map(ep -> {
int convertRate = TIME_UNIT_CONVERSION.getOrDefault(ep.timeUnit, 1);
return Objects.nonNull(ep.timeUnitValue) ? ep.coeff * ep.timeUnitValue * convertRate : 0;
}).orElse(0);
}

public static boolean compare(Criteria c1, Criteria c2) {

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,17 +18,20 @@

package org.ohdsi.circe.check.checkers;

import static org.ohdsi.circe.check.operations.Operations.match;
import static org.ohdsi.circe.cohortdefinition.DateOffsetStrategy.DateField.StartDate;

import java.util.Objects;
import org.ohdsi.circe.check.WarningSeverity;
import org.ohdsi.circe.cohortdefinition.CohortExpression;
import org.ohdsi.circe.cohortdefinition.DateOffsetStrategy;

import java.util.Arrays;
import java.util.List;
import java.util.Objects;

import static org.ohdsi.circe.check.operations.Operations.match;
import static org.ohdsi.circe.cohortdefinition.DateOffsetStrategy.DateField.StartDate;

public class ExitCriteriaDaysOffsetCheck extends BaseCheck {

private static final String DAYS_OFFSET_WARNING = "Cohort Exit criteria: Days offset from start date should be greater than 0";
private static final String DAYS_OFFSET_WARNING = "Cohort Exit criteria: %ss offset from start date should be greater than 0";

@Override
protected WarningSeverity defineSeverity() {
Expand All @@ -40,9 +43,15 @@ protected WarningSeverity defineSeverity() {
protected void check(CohortExpression expression, WarningReporter reporter) {

match(expression.endStrategy)
.isA(DateOffsetStrategy.class)
.then(s -> match((DateOffsetStrategy)s)
.when(dateOffsetStrategy -> Objects.equals(StartDate, dateOffsetStrategy.dateField) && 0 == dateOffsetStrategy.offset)
.then(() -> reporter.add(DAYS_OFFSET_WARNING)));
.isA(DateOffsetStrategy.class)
.then(s -> match((DateOffsetStrategy)s)
.when(dateOffsetStrategy -> Objects.equals(StartDate, dateOffsetStrategy.dateField) && (0 == dateOffsetStrategy.offset || 0 == dateOffsetStrategy.offsetUnitValue))
.then(dateOffsetStrategy -> {
if(0 == dateOffsetStrategy.offset){
reporter.add(String.format(DAYS_OFFSET_WARNING), "Days");
}else {
reporter.add(String.format(DAYS_OFFSET_WARNING, dateOffsetStrategy.offsetUnit));
}
}));
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -43,8 +43,11 @@ protected void checkCriteria(CorelatedCriteria criteria, String groupName, Warni
Execution addWarning = () -> reporter.add(WARNING, name);
match(criteria)
.when(c -> c.startWindow != null && ((c.startWindow.start != null
&& c.startWindow.start.days != null) || (c.startWindow.end != null
&& c.startWindow.end.days != null)))
&& c.startWindow.start.days != null) || (c.startWindow.end != null
&& c.startWindow.end.days != null))
|| ((c.startWindow.start.timeUnitValue != null)
&& (c.startWindow.end != null)
&& c.startWindow.end.timeUnitValue != null))
.then(cc -> match(cc.criteria)
.isA(ConditionEra.class)
.then(c -> match((ConditionEra)c)
Expand Down
27 changes: 18 additions & 9 deletions src/main/java/org/ohdsi/circe/check/checkers/RangeCheck.java
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
import org.ohdsi.circe.cohortdefinition.Window;

import java.util.Objects;
import java.util.Optional;

public class RangeCheck extends BaseValueCheck {
private static final String NEGATIVE_VALUE_ERROR = "Time window in criteria \"%s\" has negative value %d at %s";
Expand Down Expand Up @@ -52,19 +53,27 @@ protected void checkInclusionRules(final CohortExpression expression, WarningRep
}

private void checkWindow(Window window, WarningReporter reporter, String name) {
Optional.ofNullable(window)
.map(w -> checkEndpoint(w, name, "start"))
.ifPresent(reporter::add);

if (Objects.nonNull(window)) {
if (Objects.nonNull(window.start) && Objects.nonNull(window.start.days) && window.start.days < 0) {
reporter.add(NEGATIVE_VALUE_ERROR, name, window.start.days, "start");
}
if (Objects.nonNull(window.end) && Objects.nonNull(window.end.days) && window.end.days < 0) {
reporter.add(NEGATIVE_VALUE_ERROR, name, window.end.days, "end");
}
}
Optional.ofNullable(window)
.map(w -> checkEndpoint(w, name, "end"))
.ifPresent(reporter::add);
}

private void checkObservationFilter(ObservationFilter filter, WarningReporter reporter, String name) {
private String checkEndpoint(Window window, String name, String endpointType) {
boolean hasValid = Objects.nonNull(window.start) && Objects.nonNull(window.start.days < 0 ? window.start.days : window.start.timeUnitValue) && window.start.days < 0 ? window.start.days < 0 : window.start.timeUnitValue < 0;
return Optional.of(window)
.filter(w -> hasValid)
.map(w -> String.format(NEGATIVE_VALUE_ERROR, name, getEndpointValue(w.start), endpointType))
.orElse(null);
}
private Object getEndpointValue(Window.Endpoint endpoint) {
return Objects.nonNull(endpoint.days) ? endpoint.days : endpoint.timeUnitValue;
}

private void checkObservationFilter(ObservationFilter filter, WarningReporter reporter, String name) {
if (Objects.nonNull(filter)) {
if (filter.priorDays < 0) {
reporter.add(NEGATIVE_VALUE_ERROR, name, filter.priorDays, "prior days");
Expand Down
25 changes: 17 additions & 8 deletions src/main/java/org/ohdsi/circe/check/checkers/TimePatternCheck.java
Original file line number Diff line number Diff line change
Expand Up @@ -18,10 +18,7 @@

package org.ohdsi.circe.check.checkers;

import java.util.ArrayList;
import java.util.List;
import java.util.Map;
import java.util.Objects;
import java.util.*;
import java.util.function.Function;
import java.util.stream.Collectors;
import org.ohdsi.circe.check.WarningSeverity;
Expand Down Expand Up @@ -83,17 +80,29 @@ private String formatTimeWindow(TimeWindowInfo ti) {
}

private String formatDays(Window.Endpoint endpoint) {
return Objects.nonNull(endpoint.days) ? String.valueOf(endpoint.days) : "all";
return Objects.nonNull(endpoint.days) ? String.valueOf(endpoint.days) :
Objects.nonNull(endpoint.timeUnitValue) ? String.valueOf(endpoint.timeUnitValue) : "all";

}

private String formatCoeff(Window.Endpoint endpoint) {
return endpoint.coeff < 0 ? "before " : "after ";
}


private Integer startDays(Window window) {
return Objects.nonNull(window) && Objects.nonNull(window.start) ?
(Objects.nonNull(window.start.days) ? window.start.days : 0) * window.start.coeff : 0;
}
return Optional.ofNullable(window)
.map(w -> w.start)
.map(start -> {
int coefficient = Optional.ofNullable(start.coeff).orElse(0);
return Optional.ofNullable(start.days)
.map(days -> days * coefficient)
.orElseGet(() -> Optional.ofNullable(start.timeUnitValue)
.map(timeUnitValue -> timeUnitValue * coefficient)
.orElse(0));
})
.orElse(0);
}

class TimeWindowInfo {
private String name;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,9 +20,8 @@

import com.fasterxml.jackson.annotation.JsonProperty;
import com.fasterxml.jackson.databind.ObjectMapper;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.List;

import java.util.*;
import java.util.stream.Collectors;
import java.util.stream.IntStream;
import org.apache.commons.lang3.StringUtils;
Expand Down Expand Up @@ -351,8 +350,12 @@ public String buildExpressionQuery(CohortExpression expression, BuildExpressionQ

resultSql = StringUtils.replace(resultSql, "@cohort_end_unions", StringUtils.join(endDateSelects, "\nUNION ALL\n"));

resultSql = StringUtils.replace(resultSql, "@eraconstructorpad", Integer.toString(expression.collapseSettings.eraPad));

if(!StringUtils.isEmpty(Integer.toString(expression.collapseSettings.eraPad)) && expression.collapseSettings.eraPad != 0){
resultSql = StringUtils.replace(resultSql, "@eraconstructorpad", Integer.toString(expression.collapseSettings.eraPad));
}else{
resultSql = StringUtils.replace(resultSql, "@era_pad_unit", expression.collapseSettings.eraPadUnit);
resultSql = StringUtils.replace(resultSql, "@era_pad", Integer.toString(expression.collapseSettings.eraPadUnitValue));
}
resultSql = StringUtils.replace(resultSql, "@inclusionRuleTable", getInclusionRuleTableSql(expression));
resultSql = StringUtils.replace(resultSql, "@inclusionImpactAnalysisByEventQuery", getInclusionAnalysisQuery("#qualified_events", 0));
resultSql = StringUtils.replace(resultSql, "@inclusionImpactAnalysisByPersonQuery", getInclusionAnalysisQuery("#best_events", 1));
Expand Down Expand Up @@ -547,19 +550,24 @@ public String getWindowedCriteriaQuery(String sqlTemplate, WindowedCriteria crit
String startIndexDateExpression = (startWindow.useIndexEnd != null && startWindow.useIndexEnd) ? "P.END_DATE" : "P.START_DATE";
String startEventDateExpression = (startWindow.useEventEnd != null && startWindow.useEventEnd) ? "A.END_DATE" : "A.START_DATE";
if (startWindow.start.days != null) {
startExpression = String.format("DATEADD(day,%d,%s)", startWindow.start.coeff * startWindow.start.days, startIndexDateExpression);
startExpression = String.format("DATEADD(day,%d,%s)", startWindow.start.coeff * startWindow.start.days, startIndexDateExpression);
} else if (startWindow.start.timeUnitValue != null) {
startExpression = String.format("DATEADD(%s,%d,%s)", startWindow.start.timeUnit, startWindow.start.coeff * startWindow.start.timeUnitValue, startIndexDateExpression);
} else {
startExpression = checkObservationPeriod ? (startWindow.start.coeff == -1 ? "P.OP_START_DATE" : "P.OP_END_DATE") : null;
startExpression = checkObservationPeriod ? (startWindow.start.coeff == -1 ? "P.OP_START_DATE" : "P.OP_END_DATE") : null;
}

if (startExpression != null) {
clauses.add(String.format("%s >= %s", startEventDateExpression, startExpression));
}

if (startWindow.end.days != null) {
endExpression = String.format("DATEADD(day,%d,%s)", startWindow.end.coeff * startWindow.end.days, startIndexDateExpression);
endExpression = String.format("DATEADD(day,%d,%s)", startWindow.end.coeff * startWindow.end.days, startIndexDateExpression);
}
else if (startWindow.end.timeUnitValue != null) {
endExpression = String.format("DATEADD(%s,%d,%s)", startWindow.start.timeUnit, startWindow.end.coeff * startWindow.end.timeUnitValue, startIndexDateExpression);
} else {
endExpression = checkObservationPeriod ? (startWindow.end.coeff == -1 ? "P.OP_START_DATE" : "P.OP_END_DATE") : null;
endExpression = checkObservationPeriod ? (startWindow.end.coeff == -1 ? "P.OP_START_DATE" : "P.OP_END_DATE") : null;
}

if (endExpression != null) {
Expand All @@ -574,19 +582,23 @@ public String getWindowedCriteriaQuery(String sqlTemplate, WindowedCriteria crit
// for backwards compatability, having a null endWindow.useIndexEnd means they SHOULD use the index end date.
String endEventDateExpression = (endWindow.useEventEnd == null || endWindow.useEventEnd) ? "A.END_DATE" : "A.START_DATE";
if (endWindow.start.days != null) {
startExpression = String.format("DATEADD(day,%d,%s)", endWindow.start.coeff * endWindow.start.days, endIndexDateExpression);
startExpression = String.format("DATEADD(day,%d,%s)", endWindow.start.coeff * endWindow.start.days, endIndexDateExpression);
} else if (endWindow.start.timeUnitValue != null) {
startExpression = String.format("DATEADD(%s,%d,%s)", endWindow.start.timeUnit, endWindow.start.coeff * endWindow.start.timeUnitValue, endIndexDateExpression);
} else {
startExpression = checkObservationPeriod ? (endWindow.start.coeff == -1 ? "P.OP_START_DATE" : "P.OP_END_DATE") : null;
startExpression = checkObservationPeriod ? (endWindow.start.coeff == -1 ? "P.OP_START_DATE" : "P.OP_END_DATE") : null;
}

if (startExpression != null) {
clauses.add(String.format("%s >= %s", endEventDateExpression, startExpression));
}

if (endWindow.end.days != null) {
endExpression = String.format("DATEADD(day,%d,%s)", endWindow.end.coeff * endWindow.end.days, endIndexDateExpression);
endExpression = String.format("DATEADD(day,%d,%s)", endWindow.end.coeff * endWindow.end.days, endIndexDateExpression);
} else if (endWindow.end.timeUnitValue != null) {
endExpression = String.format("DATEADD(%s,%d,%s)", endWindow.start.timeUnit, endWindow.end.coeff * endWindow.end.timeUnitValue, endIndexDateExpression);
} else {
endExpression = checkObservationPeriod ? (endWindow.end.coeff == -1 ? "P.OP_START_DATE" : "P.OP_END_DATE") : null;
endExpression = checkObservationPeriod ? (endWindow.end.coeff == -1 ? "P.OP_START_DATE" : "P.OP_END_DATE") : null;
}

if (endExpression != null) {
Expand Down Expand Up @@ -763,9 +775,13 @@ private String getDateFieldForOffsetStrategy(DateOffsetStrategy.DateField dateFi
@Override
public String getStrategySql(DateOffsetStrategy strat, String eventTable) {
String strategySql = StringUtils.replace(DATE_OFFSET_STRATEGY_TEMPLATE, "@eventTable", eventTable);
strategySql = StringUtils.replace(strategySql, "@offset", Integer.toString(strat.offset));
if(strat.offset != 0){
strategySql = StringUtils.replace(strategySql, "@offset", Integer.toString(strat.offset));
}else {
strategySql = StringUtils.replace(strategySql, "@offsetUnitValue", Integer.toString(strat.offsetUnitValue));
strategySql = StringUtils.replace(strategySql, "@offsetUnit", strat.offsetUnit);
}
strategySql = StringUtils.replace(strategySql, "@dateField", getDateFieldForOffsetStrategy(strat.dateField));

return strategySql;
}

Expand All @@ -790,4 +806,5 @@ public String getStrategySql(CustomEraStrategy strat, String eventTable) {
}

// </editor-fold>

}
Original file line number Diff line number Diff line change
Expand Up @@ -21,11 +21,14 @@
import com.fasterxml.jackson.annotation.JsonProperty;

public class CollapseSettings {

@JsonProperty("CollapseType")
public CollapseType collapseType = CollapseType.ERA;

@JsonProperty("EraPad")
public int eraPad = 0;


@JsonProperty("CollapseType")
public CollapseType collapseType = CollapseType.ERA;
@JsonProperty("EraPad")
public int eraPad = 0;
@JsonProperty("EraPadUnit")
public String eraPadUnit;
@JsonProperty("EraPadUnitValue")
public int eraPadUnitValue = 0;

}
Original file line number Diff line number Diff line change
Expand Up @@ -30,13 +30,16 @@ public enum DateField {
StartDate,
EndDate
}

@JsonProperty("Offset")
public int offset = 0;
@JsonProperty("DateField")
public DateField dateField = DateField.StartDate;

@JsonProperty("Offset")
public int offset = 0;

@JsonProperty("OffsetUnitValue")
public int offsetUnitValue = 0;
@JsonProperty("OffsetUnit")
public String offsetUnit;

@Override
public String accept(IGetEndStrategySqlDispatcher dispatcher, String eventTable) {
return dispatcher.getStrategySql(this, eventTable);
Expand Down
19 changes: 19 additions & 0 deletions src/main/java/org/ohdsi/circe/cohortdefinition/IntervalUnit.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
package org.ohdsi.circe.cohortdefinition;

import jdk.nashorn.internal.objects.annotations.Getter;

public enum IntervalUnit {
HOUR("hour"),
MINUTE("minute"),
SECOND("second");

private final String name;

IntervalUnit(String name) {
this.name = name;
}

public String getName() {
return name;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,6 @@
public class ObservationFilter {
@JsonProperty("PriorDays")
public int priorDays;
@JsonProperty("PostDays")
@JsonProperty("PostDays")
public int postDays;
}
Loading