Support for datetime calculations in cohort definitions

[jcnamendiOdysseus]

No description provided.

[chrisknoll]

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?

[alex-odysseus]

Chris, please don't go deep into this pull request as it is a draft and incomplete @chrisknoll

[chrisknoll]

Will be easier to point out issues earlier instead of sifting through potentially thousands of lines of changes.

[chrisknoll]

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.

[chrisknoll]

You want to be extremly careful with changing the formatting of the Freemarker templates (ftl files) that are building the markdown format, since these are very sensitive to whitespacing. Unless you need to change this, I would not touch any of these.

[wivern]

This produces an NPE when window.start is not set and timeUnit is a DAY.

[TomWhite-MedStar]

@chrisknoll , if this pull request is related to OHDSI/Atlas#2886, I'm looking for advice on getting past a hurdle. the GUI looks good, and when one explicitly specifies hour or minute for a widget, the generated code looks correct for that widget. However, if there is a single reference to datetime in the JSON, then all dates that can be converted to datetime must be.

There are two areas of challenge in the current efforts to implement this:

1. Identifying that datetime is used somewhere within the cohort logic
2. Ensuring that all references to date are converted to datetime (for the parts of the CDM that support datetime).

I think this can be handled by post-processing the SQL generated by circe-be:

The following python function addresses both steps:

def Fix_OHDSI_sql_if_uses_datetime(ohdsi_sql):
    import re

    # Test whether generate SQL uses datetime.  Another option is to query the cohort JSON definition for TimeUnit of hour|minute|second
    if (re.search(r'_datetime', ohdsi_sql)):
        # Convert all references to _date to _datetime, unless CDM does not support it (e.g. for observation_period and cohort)
        step1 = re.sub(r'(?i)(?<!op_)(?<!observation_period_)(start|end|event|sort)_date\b',r'\1_datetime',ohdsi_sql)
        # Cohort final table will have datetime, but cohort table only supports date.
        # The below substitution assumes that step1 has already occured, thus the reference to datetime in the first parameter to re.sub
        step2 = re.sub(r'as cohort_definition_id, person_id, start_datetime, end_datetime',r'as cohort_definition_id, person_id, cast(start_datetime as date), cast(end_datetime as date)', step1)
        return step2
    else:
        return ohdsi_sql

If we find that the generated SQL doesn't always have a datetime reference, another option is to use regular expressions to search the JSON definition for "TimeUnit" of "hour|minute|second".

Can the circe-be function that does the SQL template generation be amended (or intercepted) to call this additional function (or its Java equivalent) before the final String is returned to the requestor? From there, SqlRender should work fine on the syntax-specific translation.

[chrisknoll]

Hi, @TomWhite-MedStar ,

I haven't dug deeply into this, as there's missing information in the PR description about what the changes are (in moderate detail), but currently has nothing yet. Second issue is the entire notion of minute/second granularity in the cohort result, it is not clear what the impact on the external tools will be if there is a unit of time other than day.

Also, looks like this has some conflicts based on recent PRs that have been merged into master.

As far as using this for your own purposes, a branch or fork approach would work for you to use this in your own environment if there is that need.

As you might imagine that this cohort definition work is central to a lot of the OHDSI tool stack, we must be extremely careful what gets put into a formal release. On the forums, I've observed support that it's a good idea (hour/minute granularity), but beyond that there are technical considerations (such as: sometimes the result query will put _datetime as an output column other times will be simply _date) that haven't been vetted, but we're making the decision on how that should work here.

So for starters, I think the PR author should include details on what the intended changes are that are being introduced in this PR (so that we have something to cross-check between the 'intended change' vs. the actual change in the 'lines of code changed'). This is important because we don't want to be left to reverse-engineer the lines of code that were changed to infer the intent of the author...it should be declared, and then we can judge the code changes based on that declared intent.

On small changes, it's pretty straight forward to declare what the intended changes of a PR is. On something larger like this, we have new test cases, enums, date calculations (I think I saw in some cases that we intended to resolve the time diffs to seconds, but in certain places I see we're accounting for sometimes hours, sometimes minutes, sometimes seconds), so I just would like to know what I should be expecting when looking through the code changes before digesting all the lines of code changes.

Re: your approach to fix sql to date or date_time via string replace
I'm not a fan of this approach, I don't know if we can assume that date-time cols are always _datetime I am thinking that it may be a cohort expression param that you specify the granularity of time for the output such that if you want it by hour, then you query all the entry events at the hour level, vs some other granularity. This is definitely something that should be thought about at the design level, but we sorta jumped right into the implementation so there wasn't much time to talk about...but we have this implementation as a 'frame of reference' that we could raise technical issues, so that is also a fine perspective to reason from.

[TomWhite-MedStar]

@chrisknoll , thanks for the lengthy reply. I am presuming, but not sure, that this PR might be for work that Odysseus is doing for me at MedStar. If so, I'm trying to get past a hurdle in that work. They have working for months, and finding that the source code is very complex - so I'm wanting to brainstorm whether there might be post-processing approach that is easier to implement and maintain.

[chrisknoll]

Thanks for your understanding, @TomWhite-MedStar . Although the codebase may be complicated, at the core it's a string-builder plus logic validator (through the checkers). This means that, at the core, it's assembling the string through the various builder functions. Introducing a notion of building the final string through a process of find/replaces might seem straightforward from an implementation perspective, but out of place from a functional perspective.

So, lets get the description of what's in the PR in the description of this PR, resolve the conflicts, evaluate if the code is doing what it intended, and then we can make implementation recommendations based on findings.