cqframework / cql-execution

A JavaScript framework for executing CQL
Apache License 2.0
65 stars 30 forks source link

Error Using Collapse Without Unit #255

Closed JSRankins closed 3 years ago

JSRankins commented 3 years ago

The CumulativeMedicationDuration library contains a function using a collapse.

define function "CumulativeDuration"(Intervals List<Interval<Date>> ):
  Sum((collapse Intervals) X return all difference in days between start of X and end of X)

This function throws an error when more than one interval is in the list (e.g., {Interval[@2010-01-01, @2010-06-01], Interval[@2010-06-02, @2010-12-31]} ) provided to the function. Stacktrace:

cqm_calculator.self.js?body=1:75 Error: Cannot create a quantity with an undefined value
    at new Quantity (browser.self.js?body=1:24952)
    at Interval.getPointSize (browser.self.js?body=1:24691)
    at collapseIntervals (browser.self.js?body=1:29895)
    at Collapse.exec (browser.self.js?body=1:29856)
    at Collapse.execute (browser.self.js?body=1:28530)
    at MultiSource.forEach (browser.self.js?body=1:32670)
    at Query.exec (browser.self.js?body=1:32538)
    at Query.execute (browser.self.js?body=1:28530)
    at Sum.exec (browser.self.js?body=1:25554)
    at Sum.execute (browser.self.js?body=1:28530)
    at FunctionRef.exec (browser.self.js?body=1:32980)
    at FunctionRef.execute (browser.self.js?body=1:28530)
    at browser.self.js?body=1:28548
    at Array.map (<anonymous>)
    at Greater.execArgs (browser.self.js?body=1:28547)

CQL supports the use of collapse with or without a specified unit for the collapse. The function in the library does not specify a unit. Given the stacktrace, we speculate that this is the issue but question if that behavior is correct based on CQL.

cmoesel commented 3 years ago

@JSRankins - I just took a look at our code and we do have several unit tests that specifically test collapse of DateTime intervals without specifying a per unit. The tests all pass -- so there is some edge case happening here. I'll look into it further -- but it would help to know if there is anything unusual about the intervals being passed in. Are any of them null? Do any have null boundaries? If so, open null or closed null? Etc...

JSRankins commented 3 years ago

Hi @cmoesel No nulls. The inputs are actually datetime based (both using a Medication Order's relevantPeriod), but the CumulativeDuration function invokes MedicationPeriod function which uses a MedicationOrderPeriod function that strips off the time portion and ultimately passes the data back to the CumulativeDuration function as dates.

define function "MedicationOrderPeriod"(Order "Medication, Order" ):
  if Order.relevantPeriod.low is null and Order.authorDatetime is null then
    null
  else if Order.relevantPeriod.high is not null then
    Interval[date from Coalesce(Order.relevantPeriod.low, Order.authorDatetime), date from end of Order.relevantPeriod]
  .....

Intervals used: Interval[@2012-04-01T08:00, @2012-10-13T08:15] Interval[@2012-10-14T08:00, @2012-10-16T08:15]

cmoesel commented 3 years ago

Hmmm... OK. Thanks! I'll try testing w/ those exact values to see what happens!

cmoesel commented 3 years ago

OK. I was able to reproduce this. Thanks for the extra information, @JSRankins. It looks like we have a bug when collapsing intervals of Date or intervals of Time when no per is provided (but collapsing intervals of DateTime with no per works fine). I have a local fix in place, but I need to test it some more before it's ready.

JSRankins commented 3 years ago

That confirms what we were seeing @cmoesel. When per is added to the CQL logic, https://github.com/cqframework/cql-execution/issues/256 arises. That issue is against the 2.2.0 source code, so this issue may be have been resolved in a later release - something we are wanting to confirm.

JSRankins commented 3 years ago

Tagging @brynrhodes on this since this issue impacts the QDM version of CumulativeMedicationDuration library. Bryn, sounds like @cmoesel is working on updating the code, but depending on timing for release and when something might be able to get into Bonnie, thoughts about using a workaround for this logic in the CMD library?

cmoesel commented 3 years ago

This fix is now available in the cql-execution 2.3.3 release.