FirelyTeam / firely-cql-sdk

BSD 3-Clause "New" or "Revised" License
31 stars 18 forks source link

Unit conversion is not adhering to the specification #144

Open EvanMachusak opened 10 months ago

EvanMachusak commented 10 months ago

Our SDK performs unit equivalency for syntactic date units by normalizing them immediately to their UCUM counterparts.

For example, CqlDate line 208

This behavior is contrary to the Author's Guide §4.2.1 Quantities.

In all cases, we should preserve the original unit and instead whenever it is required to compare units as equal or equivalent, we should invoke a not yet implemented UnitComparer that will compare UCUM and syntactic units per the specification.

Here's some CQL that could reproduce it:

define function "Is year precision"(quantity System.Quantity): quantity.unit = 'a'
define "Year equal to 'a'": "Is year precision"(1 year)

In our current implementation, Year equal to 'a' returns true where pursuant to the specification, it should return false:

1 year is not equal (=) to 1 'a' (defined in UCUM as 365.25 'd'), but it is equivalent (~) to 1 'a'.

EvanMachusak commented 10 months ago

Note:

define "Alpha": 1 day = 1 'd'
define "Beta": (1 day).unit = 'd'

"Alpha" is true in both the JavaScript and Java engines. "Beta" is false in both the JavaScript and Java engines.

My proposed fix for this problem would result in "Beta" returning true, since we would do all unit comparisons with unit comparers, and day would be equal to 'd'.

Relevant zulip discussion thread here.

ewoutkramer commented 10 months ago

That's right, in the current SDK Quantity I do not normalize, but I have instead introduced a system specifically for calendar durations (since they don't exist in UCUM).

https://github.com/FirelyTeam/firely-net-sdk/blob/develop/src/Hl7.Fhir.Base/ElementModel/Types/Quantity.cs

   public static Quantity ForCalendarDuration(decimal value, string calendarUnit)
        {
            return calendarUnit is not null
                ? new Quantity(value, calendarUnit, QuantityUnitSystem.CalendarDuration)
                : throw new ArgumentNullException(nameof(calendarUnit));
        }

Here, we introduced a QuantityUnitSystem.CalendarDuration - which also means that this CQL Quantity type can have a system that is not UCUM.

This way, I don't have to normalize immediately, and can keep track of the "actual" unit.

ewoutkramer commented 2 months ago

Maybe we should use this system for it, which is specifically for calendar units: https://build.fhir.org/ig/HL7/FHIRPath/CodeSystem-calendar-units.html