FHIR Chat · Date/Time comparison · fhirpath

Stream: fhirpath

Topic: Date/Time comparison

John Timm (May 04 2020 at 20:32):

@Bryn Rhodes From, the FHIRPath spec (http://hl7.org/fhirpath/N1/#comparison):

"If one value is specified to a different level of precision than the other, the result is empty ({ }) to indicate that the result of the comparison is unknown."

and (http://hl7.org/fhirpath/N1/#datetime-equality):

"If one input has a value for the precision and the other does not, the comparison stops and the result is empty ({ });"

I take this to mean that if we are comparing a fully-specified Date and a fully-specified DateTime as in the FHIRPath spec test testDateTimeGreaterThanDate:

now() > Patient.birthDate

or the FHIRPath spec test testNow1:

Patient.birthDate < now()

Then the result should be empty ({}) because a fully-specified Date will never have the same precision as a fully-specified DateTime. However, the test suite thinks that both of these examples should return true.

Bryn Rhodes (May 05 2020 at 16:10):

Hi @John Timm , for date/time comparisons, the comparisons proceeds from years down, so if the comparison results in true or false at any specified precision, then that will be the result. Only in the case that comparison proceeds beyond the specified precision of one of the inputs will the result be null. Does that help?

John Timm (May 05 2020 at 17:21):

@Bryn Rhodes thanks for the explanation. Do these examples adequately capture the algorithm?

1970-01-01 < 2020-01-01
1970 < 2020 = true (stop) return true

1970-01-01 < 1970-02-01
1970 < 1970 = false
01 < 02 = true (stop) return true

1970-01-01 < 1970-01-01
1970 < 1970 = false
01 < 01 = false
01 < 01 = false (stop) return false


1970-01-01 < 1970-01-01T00:00:00Z
1970 < 1970 = false
01 < 01 = false
01 < 01 = false
NULL < 00 = not comparable, (stop) return empty

Aaron Nash (May 05 2020 at 17:38):

For what it's worth, I too read the spec the way you originally did John. It makes sense to me it should be the way that Bryn describes but it's easy to mis(?)-read the spec. Particularly, "If one value is specified to a different level of precision than the other, the result is empty ({ }) to indicate that the result of the comparison is unknown."

Bryn Rhodes (May 06 2020 at 21:24):

@Aaron Nash , yes, those examples accurately reflect the behavior. And also agree, it's worth a clarification, can I trouble you to submit that as a tracker as well? Would be good to include that example as a test case too.

Aaron Nash (May 07 2020 at 19:23):

https://jira.hl7.org/browse/FHIR-27055

Ewout Kramer (Jul 14 2020 at 15:01):

I implemented the algorithm mentioned above, but I fail some tests in the tests-fhir-r4.xml file, e.g.

Patient.birthDate != @1974-12-25T12:34:00-10:00 (by the way, birthDate = 1974-12-25).

Clearly, based on the above discussion, this should return empty? The test expects an output of true.

Grahame Grieve (Jul 17 2020 at 10:55):

hmm where did you get this from?

Grahame Grieve (Jul 17 2020 at 10:55):

the current master for the test is here:

Grahame Grieve (Jul 17 2020 at 10:56):

https://github.com/FHIR/fhir-test-cases/blob/master/r4/fhirpath/tests-fhir-r4.xml

Ewout Kramer (Jul 20 2020 at 10:08):

I got these from http://hl7.org/fhirpath/N1/tests.html and https://github.com/HL7/FHIRPath/blob/master/tests/r4/tests-fhir-r4.xml.

Ewout Kramer (Jul 20 2020 at 10:09):

Seems we have too many copies ;-)

Grahame Grieve (Jul 20 2020 at 11:35):

I didn't find the test case you asked about in what I think is the master?

Ewout Kramer (Jul 20 2020 at 11:59):

That's right, it is corrected in the master branch of "fhir-test-cases". It's just hard to know which is the master file, since the other two links I mentioned also seemed pretty authorative.

Ewout Kramer (Jul 22 2020 at 09:06):

@Bryn Rhodes @Grahame Grieve @Lee Surprenant - it seems we have two forks of the test files, which have both been edited.

These are the forks I am looking at:

• https://github.com/HL7/FHIRPath/blob/master/tests/r4/tests-fhir-r4.xml. Let's call this "Bryn's testfile"
• https://github.com/FHIR/fhir-test-cases/blob/master/r4/fhirpath/tests-fhir-r4.xml. Let's call this "Grahame's testfile"

"Bryn's testfile" Has been edited until end of 2019, Grahame's file has been updated in january 2020, so is newer.

Bryn's file includes edits to make the test names unique (very much appreciated!), which are missing in Grahame's. E.g."testLiteralDate" has numerous repeats, and in Bryn's file these have been made unique.

Grahame's file seem to have "uncorrected" some of the tests:

E.g. in Bryn's file has this test:

' 1 \'wk\''.toQuantity() = 1 'wk'. Which I think is correct, in Grahame's this has been changed to

' 1 \'wk\''.toQuantity() = 1 week. Which I think is wrong, since it is converting the definite time quantity wk from UCUM into the calender quantity from CQL.

Grahame's file does have some corrections of incorrect tests in Bryn's though like this one:

name !~ name (true)

I do see Grahame has committed changes to Bryn's file too, so he probably knows about both files.

Can we try to trace what is going on?

Grahame Grieve (Jul 22 2020 at 09:54):

I certainly didn't 'uncorrect' anything.

Grahame Grieve (Jul 22 2020 at 09:55):

I do want the master to be the one in fhir-test-cases - it's in with all the other tests, and suitable for integration into build pipelines, and PRs and so forth

Grahame Grieve (Jul 22 2020 at 09:55):

I can't comment about the specific differences

Ewout Kramer (Jul 22 2020 at 12:11):

Grahame Grieve said:

I certainly didn't 'uncorrect' anything.

Then we have to conclude that the files forked before these changes were made, because I think the original tests had exactly the same test for week as yours has now, Grahame.

Ewout Kramer (Jul 22 2020 at 12:12):

I do remember, that at first, '1 week = 1wk'.

Ewout Kramer (Jul 22 2020 at 12:12):

This was changed in the normative edition.

Ewout Kramer (Jul 22 2020 at 12:14):

Grahame Grieve said:

I do want the master to be the one in fhir-test-cases - it's in with all the other tests, and suitable for integration into build pipelines, and PRs and so forth

Yes, I noticed, very useful.

Ewout Kramer (Jul 22 2020 at 12:14):

But I can imagine the FhirPath spec has its set of tests too. And indeed it has, if you navigate to the FhirPath spec, there is a menu item "Tests", which lead to yet another version of the file (which has been corrected by Lee).

Grahame Grieve (Jul 22 2020 at 12:24):

the other problem I had with the tests there is that they are tied to a specific version of FHIR.

Grahame Grieve (Jul 22 2020 at 12:24):

we could correct the FHIRPath tests page to link to a list of known test suites

Ewout Kramer (Jul 22 2020 at 12:24):

yes! that's painful.

Ewout Kramer (Jul 22 2020 at 12:25):

since the releases of FhirPath & FHIR are out-of-sync.

Grahame Grieve (Jul 22 2020 at 12:25):

yes. that's why I moved them to the test cases

Ewout Kramer (Jul 22 2020 at 12:27):

But they are under r4/. you'd like to have them one directory up.

Ewout Kramer (Jul 22 2020 at 12:27):

which is possible for the tests that don't need FHIR instance data. There are actually quite a few of these.

Ewout Kramer (Jul 22 2020 at 12:29):

One thing Marco and I have been considering is having a bit of data that has all the features you need - but is against a "test" model, so not against FHIR data. Another option is to use FHIR instances that only use elements that don't differ over r3/r4/r5

Grahame Grieve (Jul 22 2020 at 12:30):

I'm open to either of them, though I don't know that it's a good investment to build a different object model that only exists to support the tests

Paul Lynch (Jul 22 2020 at 19:23):

If FHIRPath is really an independent thing, I think it should have its own tests which have as little reference to FHIR as possible.

Bryn Rhodes (Jul 22 2020 at 20:06):

Or only reference Normative content in R4?

Bryn Rhodes (Jul 22 2020 at 20:06):

I think that's where most of there are anyway

Grahame Grieve (Jul 22 2020 at 20:07):

in theory it could have it's own tests but since it doesn't define any content, that's a bit difficult

Bryn Rhodes (Jul 22 2020 at 20:08):

Right, and there aren't any "selector" constructs, so you really _can't_ test a lot of FHIRPath without picking some content model.

Bryn Rhodes (Jul 22 2020 at 20:09):

And R4 normative is a good a candidate as any (especially given the tests are already written against it:))

Bryn Rhodes (Jul 22 2020 at 20:09):

I agree the file at https://github.com/FHIR/fhir-test-cases/blob/master/r4/fhirpath/tests-fhir-r4.xml should be the source of truth, I don't think I knew about that when I was making the changes as part of the normative publication track though.

Bryn Rhodes (Jul 22 2020 at 20:10):

So that's my bad, I'm happy to reconcile those two and submit the updated cases as a PR there and an Errata report to the spec

Paul Lynch (Jul 22 2020 at 20:10):

Bryn Rhodes said:

Right, and there aren't any "selector" constructs, so you really _can't_ test a lot of FHIRPath without picking some content model.

fhirpath.js got pretty far without adding a FHIR context model. I agree there are some things that need a model, but something small could probably be created to test that.

Bryn Rhodes (Jul 22 2020 at 20:11):

Do you have those content model independent test cases expressed in the FHIRPath test case format?

Paul Lynch (Jul 22 2020 at 20:31):

fhirpath.js (in addition to its own tests) uses tests-fhir-r4.xml and four resource files (observation-example.xml, etc.), but turns them into a yaml format we use for the other tests we wrote. In that yaml format, we have disabled certain tests and groups of tests for things we do not yet support. So, it would not be trivial to pull out which ones need a model and which ones don't, but doable. Few of the fhirpath.js tests are using a model (though that is maybe not surprising given that support for models was added late) - and it looks like none of the non-disabled tests from test-fhir-r4.xml are using a model (though they are using the four test resource files).

Bryn Rhodes (Jul 22 2020 at 20:38):

So what makes the most sense to me then is to continue on the path of reconciling the fhir-test-cases and FHIRPath published test case files.

Bryn Rhodes (Aug 12 2020 at 05:58):

Okay, I've reconciled the differing test files. Tests that will most likely need to be looked at:
4) testDivide5 was changed from 1.2 / 1.8 = 0.67 to 1.2 / 1.8 = 0.66666667 per the resolution to this tracker: https://jira.hl7.org/browse/FHIR-19356

5) testQuantityLiteralWeekToString was changed from 1 week.toString() resulting in 1 '{week}' (in the FHIRPath version of the tests) to 1 week.toString() resulting in 1 'week' per the resolution to this tracker: https://jira.hl7.org/browse/FHIR-21606

See the resolution to this tracker for more complete details: https://jira.hl7.org/browse/FHIR-28242

Bryn Rhodes (Aug 12 2020 at 05:59):

I've submitted a PR against fhir-test-cases with the updated test cases: https://github.com/FHIR/fhir-test-cases/pull/67

Bryn Rhodes (Aug 12 2020 at 06:02):

@Ewout Kramer , @Lee Surprenant, @Paul Lynch , @Brian Postlethwaite , @Grahame Grieve , when you get a chance, please verify these changes, I tried to commit them in a way that would make very clear what was applied, the first commit just updates all the names, the second commit reconciles differences between the tests. And the summary of J#28242 lists everything that changed. I also added a note to the file to make clear that the source of truth is now the r5 tests in fhir-test-cases.

Paul Lynch (Aug 12 2020 at 12:13):

1.2 / 1.8 = 0.66666667 assumes a step size of 10^-8. 4.1.4. Decimal says, "Implementations can provide support for larger decimals and higher precision, but must provide at least the range and precision defined here." If some implementation supports 10^-9, then 1.2/1.8 would be 0.666666667 (one additional "6"). Does the = operator then need to change to say that comparison is made with the operands rounded to 8 places after the decimal?

Lee Surprenant (Aug 25 2020 at 13:25):

@Bryn Rhodes finally had a look at this and left a suggestion to fix an issue on the PR

Bryn Rhodes (Aug 25 2020 at 20:39):

Thanks @Paul Lynch and @Lee Surprenant . For the decimal rounding issue, I wonder if we should introduce a .round() into the test case, rather than changing the definition of equal?

Grahame Grieve (Aug 26 2020 at 04:28):

@Bryn Rhodes I'm working on this, and I have plenty of failing tests. The first issue I have is with this test

Grahame Grieve (Aug 26 2020 at 04:28):

  <test name="testDateNotEqualTimezoneOffsetBefore" inputfile="patient-example.xml"><expression>Patient.birthDate != @1974-12-25T12:34:00-10:00</expression><output type="boolean">true</output></test>

Grahame Grieve (Aug 26 2020 at 04:28):

the relevant text in the specification is:

DateTime: must be exactly the same, respecting the timezone offset (though +00:00 = -00:00 = Z)

Grahame Grieve (Aug 26 2020 at 04:30):

but your test presumes something not stated: that if the DateTimes are not exactly the same, then the comparison is false, where as we have said in the past that it should only be false if you are sure that the dates are not the same (e.g. different precision)

Bryn Rhodes (Aug 26 2020 at 04:32):

Agreed if the precisions are different that should be empty

Grahame Grieve (Aug 26 2020 at 04:35):

right. because that's what this test says:

Grahame Grieve (Aug 26 2020 at 04:35):

        <test name="testDateNotEqual" inputfile="patient-example.xml"><expression>Patient.birthDate != @1974-12-25T12:34:00</expression></test>

Grahame Grieve (Aug 26 2020 at 04:36):

so I'm going to fix the test "testDateNotEqualTimezoneOffsetBefore"

Grahame Grieve (Aug 26 2020 at 04:49):

I don't see how this test can pass:

        <test name="testStringQuantityDayLiteralToQuantity" inputfile="patient-example.xml"><expression>'1 day'.toQuantity() = 1 '{day}'</expression><output type="boolean">true</output></test>

Grahame Grieve (Aug 26 2020 at 04:51):

This should pass:

        <test name="testStringQuantityDayLiteralToQuantity" inputfile="patient-example.xml"><expression>'1 day'.toQuantity() = 1 'd'</expression><output type="boolean">true</output></test>

Grahame Grieve (Aug 26 2020 at 04:53):

And I don't see how an implementation can pass both of these tests:

        <test name="testQuantityLiteralWkToString" inputfile="patient-example.xml"><expression>1 'wk'.toString()</expression><output type="string">1 'wk'</output></test>
        <test name="testQuantityLiteralWeekToString" inputfile="patient-example.xml"><expression>1 week.toString()</expression><output type="string">1 'week'</output></test>

Grahame Grieve (Aug 26 2020 at 05:32):

@Bryn Rhodes who can release a new version of the ucum code?

Grahame Grieve (Aug 26 2020 at 05:35):

because I made a PR for you approve and release https://github.com/FHIR/Ucum-java/pull/16

Grahame Grieve (Aug 26 2020 at 05:58):

@Bryn Rhodes I don't understand this test:

        <test name="testEquality7" inputfile="patient-example.xml"><expression>(1 | 1) = (1 | 2 | {})</expression></test>

Grahame Grieve (Aug 26 2020 at 05:59):

why would this have no outcome?

Grahame Grieve (Aug 26 2020 at 05:59):

from the spec:

In other words, this function returns the distinct list of elements from both inputs. For example, consider two lists of integers A: 1, 1, 2, 3 and B: 2, 3:

A union B // 1, 2, 3
A union { } // 1, 2, 3

Grahame Grieve (Aug 26 2020 at 06:00):

(1 | 1) = {1}
(1 | 2 | {} ) = {1 ,2}

Grahame Grieve (Aug 26 2020 at 06:00):

and then equality:

Grahame Grieve (Aug 26 2020 at 06:00):

If both operands are collections with multiple items:
Each item must be equal
Comparison is order dependent

Grahame Grieve (Aug 26 2020 at 06:00):

so the comparison is false.

Grahame Grieve (Aug 26 2020 at 07:06):

also, I don't understand the logic here:

<test name="testNEquality23" inputfile="patient-example.xml"><expression>1.2 / 1.8 != 0.67</expression><output type="boolean">true</output></test>

what do you think would be true, and why according to the spec?

Grahame Grieve (Aug 26 2020 at 09:54):

Another one: This cannot be true:

        <test name="testDateTimeGreaterThanDate" inputfile="patient-example.xml"><expression>now() &gt; Patient.birthDate</expression><output type="boolean">true</output></test>

I'm changing it to

        <test name="testDateTimeGreaterThanDate" inputfile="patient-example.xml"><expression>now() &gt; Patient.birthDate</expression></test>
        <test name="testDateGreaterThanDate" inputfile="patient-example.xml"><expression>today() &gt; Patient.birthDate</expression><output type="boolean">true</output></test>

Bryn Rhodes (Aug 26 2020 at 14:25):

@Grahame Grieve , working on a 1.0.3 Ucum-java release

Bryn Rhodes (Aug 26 2020 at 14:26):

Agree on testEquality7, that should be false, not empty.

Bryn Rhodes (Aug 26 2020 at 14:27):

On testNEquality23, 1.2 / 1.8 should be 0.66666667 (to however many decimal places the implementation supports, but at least 8), so it would not compare equal to 0.67.

Bryn Rhodes (Aug 26 2020 at 14:31):

On testDateTimeGreaterThanDate, now() > Patient.birthDate would be true, so long as the value of Patient.birthDate was at most yesterday, because the comparison proceeds from years down.

Bryn Rhodes (Aug 26 2020 at 15:23):

New Ucum release 1.0.3 available: https://github.com/FHIR/Ucum-java/releases/tag/v1.0.3

Grahame Grieve (Aug 26 2020 at 19:45):

thanks for the new UCUM release

Grahame Grieve (Aug 26 2020 at 19:47):

now() > Patient.birthDate would be true, so long as the value of Patient.birthDate was at most yesterday, because the comparison proceeds from years down

I do not see this written in the spec. What I see is 'strictly greater than'

Grahame Grieve (Aug 26 2020 at 19:47):

1.2 / 1.8 should be 0.66666667

no. 1.2000000/1.8000000 should be 0.66666667; 1.2/1.8 should be 0.67.

Grahame Grieve (Aug 26 2020 at 19:48):

which is what the reference implementation does

Paul Lynch (Aug 26 2020 at 22:21):

Grahame Grieve said:

1.2/1.8 should be 0.67.

I don't see anything about rounding or precision stated in the Math section.

Grahame Grieve (Aug 26 2020 at 22:24):

no. I don't think that's from the spec

Paul Lynch (Aug 26 2020 at 22:39):

If you are reporting a result, I agree that the final number should have the same number of significant digits. The trouble is that I don't think FHIRPath knows whether the result is 'final' or intermediate in some larger calculation.

Grahame Grieve (Aug 26 2020 at 22:43):

no but my implementation does

Grahame Grieve (Aug 27 2020 at 01:52):

@Bryn Rhodes

Please check the changes at https://github.com/FHIR/fhir-test-cases/pull/70

Grahame Grieve (Aug 27 2020 at 01:54):

There's one set of changes in there that are driven by a limitation I can't deal with right now: the hapi fhir core library can't deal with a time that has just hours. In fact, that's really a bad thing for FHIRPath to require dealing with. But for now I've worked around it by changing the tests. The others are genuine issues in the tests

Bryn Rhodes (Sep 02 2020 at 18:54):

Agree with the changes, except for:

• testStringQuantityDayLiteralToQuantity, that should be empty
• testQuantityLiteralWeekToString, why would that convert the calendar unit to a UCUM unit?
• testNEquality22, testNEquality23, and testDivide5, we didn't want to prescribe support for maintaining significant digits, so can we add a .round() to these tests?

Bryn Rhodes (Sep 02 2020 at 19:23):

https://github.com/FHIR/fhir-test-cases/pull/71

Paul Lynch (Sep 02 2020 at 19:50):

Left comment on pull request.

Ewout Kramer (Sep 07 2020 at 12:39):

My tests are failing on encode, decode, escape, unescape and trim. I have looked at the normative version of FhirPath and the latest fhirpath addendum to FHIR, but cannot find the definitions of these functions . Where should I look?

Ewout Kramer (Sep 07 2020 at 12:40):

All these new encode/decode tests (the group testEncodeDecode) do not carry a name per test, whereas the others do. Can we add names to them?

Ewout Kramer (Sep 07 2020 at 12:48):

We agreed that this test is wrong: "testIntegerBooleanNotTrue",

Ewout Kramer (Sep 07 2020 at 12:49):

And I don't think these are correct: "testDateNotEqualTimezoneOffsetBefore", "testDateNotEqualTimezoneOffsetAfter", "testDateNotEqualUTC"

Ewout Kramer (Sep 07 2020 at 12:53):

This is from my notes, I'll have to check on why. I see a reference to https://jira.hl7.org/browse/FHIR-27033 and https://chat.fhir.org/login/#narrow/stream/179266-fhirpath/topic/Singleton.20Evaluation.20of.20Collections in reference to "testIntegerBooleanNotTrue".

Ewout Kramer (Sep 07 2020 at 12:56):

The "testDateNotEqual" etc:

One example is this:

   <test name="testDateNotEqual" inputfile="patient-example.xml">
      <expression>Patient.birthDate != @1974-12-25T12:34:00</expression>
    </test>

For reference, Patient.birthDate is:

<birthDate value="1974-12-25">
        <extension url="http://hl7.org/fhir/StructureDefinition/patient-birthTime">
            <valueDateTime value="1974-12-25T14:35:45-05:00"/>
        </extension>
    </birthDate>

The datetimes have the same precision, but they are clearly not the same, hence I think this should result in true.

Ewout Kramer (Sep 07 2020 at 12:57):

(deleted)

Ewout Kramer (Sep 07 2020 at 12:58):

Or is this a timezone issue?

Grahame Grieve (Sep 07 2020 at 13:22):

no - this is an extension issue. There is no instruction to the FHIRPath engine to pick up the birth time extension

Grahame Grieve (Sep 07 2020 at 13:23):

@Bryn Rhodes and I added encode, etc to meet some use cases we had. I believe Bryn created a task to add them to a future version of FHIRPath

Grahame Grieve (Sep 07 2020 at 13:24):

I think we should have labelled those tests with a version indicator

Ewout Kramer (Sep 07 2020 at 14:39):

Can I assume the hex encoding/decoding turns the string into hex via UTF8?

Ewout Kramer (Sep 07 2020 at 14:56):

Actually, the base64 is also using UTF8.

Ewout Kramer (Sep 07 2020 at 16:03):

There's variance on how to deal with the padding chars in urlbase64. I propose stripping '=', but other variants are replacing '=' by '.'

Ewout Kramer (Sep 07 2020 at 16:04):

(and the current test uses the padding char '=', but that then needs to be escaped, which is really defeating the point of urlbase64)

Grahame Grieve (Sep 07 2020 at 19:45):

@Bryn Rhodes ?

Bryn Rhodes (Sep 07 2020 at 19:46):

Documentation is on a branch here: http://build.fhir.org/ig/HL7/FHIRPath/branches/feature-27757-additional-string-functions/

Bryn Rhodes (Sep 07 2020 at 19:46):

Working through this feedback now.

Bryn Rhodes (Sep 07 2020 at 19:47):

I've added names to the tests and a version attribute to the schema and set it for the new tests (to 2.1.0).

Bryn Rhodes (Sep 07 2020 at 19:47):

Now working through the reported discrepancies

Bryn Rhodes (Sep 07 2020 at 19:54):

Agreed on testIntegerBooleanNotTrue, that should result in an error, since a boolean-valued function is being invoked on a non-boolean-valued argument. Same with testIntegerBooleanNotFalse, so I've added invalid="semantic" to those tests.

Ewout Kramer (Sep 07 2020 at 20:01):

I'll take a look at what is exactly the matter with those date tests, but I think:

 <test name="testDateNotEqualTimezoneOffsetBefore" inputfile="patient-example.xml">
      <expression>Patient.birthDate != @1974-12-25T12:34:00-10:00</expression>
      <output type="boolean">true</output>
    </test>
    <test name="testDateNotEqualTimezoneOffsetAfter" inputfile="patient-example.xml">
      <expression>Patient.birthDate != @1974-12-25T12:34:00+10:00</expression>
      <output type="boolean">true</output>
    </test>
    <test name="testDateNotEqualUTC" inputfile="patient-example.xml">
      <expression>Patient.birthDate != @1974-12-25T12:34:00Z</expression>
      <output type="boolean">true</output>
    </test>

where the Patient.birthDate has precision upto the date should all have an empty result.

Grahame Grieve (Sep 07 2020 at 20:01):

no because some of them you know that it can't the same date

Ewout Kramer (Sep 07 2020 at 20:05):

<birthDate value="1974-12-25">. All of these dates are on the 25th, right? (it's easy to make a mistake here, so just double checking ;-))

Bryn Rhodes (Sep 07 2020 at 20:24):

I think the tests are expecting that the birthTime is being considered, so that's the first thing to get settled

Bryn Rhodes (Sep 07 2020 at 20:40):

I think since what we're testing is datetime equality, rather than extension access, I should just put the literal date in the test, rather than reading it from patient.

Ewout Kramer (Sep 07 2020 at 20:49):

good idea. That's actually useful for other tests too. Once you've determined that access to instance data by path (and some specific operations) work, we don't have to refer to the model anymore -> makes the tests more reusable across versions....

Grahame Grieve (Sep 07 2020 at 21:18):

I'm passing the tests and not considering birthTime.

Grahame Grieve (Sep 07 2020 at 21:19):

Changing these tests is tiresome.. I'll be spending several hours in my date time comparison stuff. I really thought these tests align with the spec now

Ewout Kramer (Sep 08 2020 at 06:59):

Let's figure out why you pass, and I fail first. I could be wrong too. The reason I fail the test is that the precision is different, when this starts to matter: the dates are exactly the same, so you cannot conclude they are unequal, unless you have more precision. Since there is no more precision available in one of the two operands, I return empty.

Ewout Kramer (Sep 08 2020 at 07:01):

I did this based on this piece of text:

If the values are the same, comparison proceeds to the next precision; if the values are different, the comparison stops and the result is false. If one input has a value for the precision and the other does not, the comparison stops and the result is empty ({ });

Ewout Kramer (Sep 08 2020 at 07:47):

Documentation is on a branch here: http://build.fhir.org/ig/HL7/FHIRPath/branches/feature-27757-additional-string-functions/

Should I provide feedback on this text here or as tracker items?

Grahame Grieve (Sep 08 2020 at 08:41):

I think the dates are different before the precision differs

Ewout Kramer (Sep 08 2020 at 08:54):

Then you are right, but I just see them all being "1974-12-25"

Grahame Grieve (Sep 08 2020 at 10:17):

which ones exactly are we talking about?

Ewout Kramer (Sep 08 2020 at 10:45):

 <test name="testDateNotEqualTimezoneOffsetBefore" inputfile="patient-example.xml">
      <expression>Patient.birthDate != @1974-12-25T12:34:00-10:00</expression>
      <output type="boolean">true</output>
    </test>
    <test name="testDateNotEqualTimezoneOffsetAfter" inputfile="patient-example.xml">
      <expression>Patient.birthDate != @1974-12-25T12:34:00+10:00</expression>
      <output type="boolean">true</output>
    </test>
    <test name="testDateNotEqualUTC" inputfile="patient-example.xml">
      <expression>Patient.birthDate != @1974-12-25T12:34:00Z</expression>
      <output type="boolean">true</output>
    </test>

Ewout Kramer (Sep 08 2020 at 10:45):

<birthDate value="1974-12-25">

Ewout Kramer (Sep 08 2020 at 11:25):

For the encoding of human time units, it seems "1 week" = 1 'week'. But there is also a test:

    <test name="testStringQuantityDayLiteralToQuantity" inputfile="patient-example.xml">
      <expression>'1 day'.toQuantity() = 1 '{day}'</expression>
      <output type="boolean">true</output>
    </test>

where "1 day" = 1 '{day}'. I am assuming now that should be 1 'day'?

Ewout Kramer (Sep 08 2020 at 11:26):

(deleted)

Ewout Kramer (Sep 08 2020 at 11:27):

(deleted)

Ewout Kramer (Sep 08 2020 at 11:34):

Actually based on this (https://chat.fhir.org/#narrow/stream/179266-fhirpath/topic/Date.2FTime.20comparison.20vs.20equality) discussion, it should now be '1 day'.toQuantity() = 1 'd'.

Ewout Kramer (Sep 08 2020 at 11:34):

and even '1 week'.toQuantity() = 1 'wk'.

Ewout Kramer (Sep 08 2020 at 12:31):

So, I did that now. To be complete, this is what I have done:

image.png

Ewout Kramer (Sep 08 2020 at 12:33):

Is this correct?

  <test name="testRound2" inputfile="patient-example.xml">
      <expression>3.14159.round(3) = 2</expression>
      <output type="boolean">true</output>
    </test>

Ewout Kramer (Sep 08 2020 at 13:31):

Agreed on testIntegerBooleanNotTrue, that should result in an error, since a boolean-valued function is being invoked on a non-boolean-valued argument.

So, for not(), we do not do singleton evaluation of the collection?

I see the spec says:

In general, when a collection is passed as an argument to a function or operator that expects a single item as input, the collection is implicitly converted to a singleton as follows:

Is the (0) passed here "as an argument to the function", or not (because it is the focus, not the argument) ?

Ewout Kramer (Sep 08 2020 at 13:35):

@Bryn Rhodes , these new tests don't seem to align with your answers on Singleton Evaluation:

 <group>
    name="from-Zulip">
    <test inputfile="patient-example.xml">
      <expression>(true and 'foo').empty()</expression>
      <output type="boolean">true</output>
    </test>
    <test inputfile="patient-example.xml">
      <expression>(true | 'foo').allTrue()</expression>
      <output type="boolean">false</output>
    </test>
  </group>

(true and 'foo') is 'true'. So (true and 'foo').empty() is false.

The second one, you said, was an error. That would be a comparable discussion to (0).not(), so I'll wait for that to settle ;-)

Lee Surprenant (Sep 08 2020 at 17:22):

Ewout Kramer said:

 <test name="testDateNotEqualTimezoneOffsetBefore" inputfile="patient-example.xml">
      <expression>Patient.birthDate != @1974-12-25T12:34:00-10:00</expression>
      <output type="boolean">true</output>
    </test>
    <test name="testDateNotEqualTimezoneOffsetAfter" inputfile="patient-example.xml">
      <expression>Patient.birthDate != @1974-12-25T12:34:00+10:00</expression>
      <output type="boolean">true</output>
    </test>
    <test name="testDateNotEqualUTC" inputfile="patient-example.xml">
      <expression>Patient.birthDate != @1974-12-25T12:34:00Z</expression>
      <output type="boolean">true</output>
    </test>

FWIW our impl seems to match Ewouts. The dates are the same and so it goes to time and only one has a time so they are not comparable => returns empty and not true.
If the tests expect the impl to know about this special extension, then that explains it, but our impl not so clever. Agree changing it to use literals will be better than testing this implied birthDate extension processing thing (is that even specified somewhere?)

Bryn Rhodes (Sep 08 2020 at 17:31):

@Grahame Grieve, @Ewout Kramer , some of the tests have a mode="strict" attribute, what is the intent of that attribute? How do your test runners interpret that?

Bryn Rhodes (Sep 08 2020 at 17:33):

There's also a checkOrderedFunctions="true" and ordered="true". I'm assuming this means that the test runner will check whether the results are ordered, and that these are different representations for the same attribute. Is that correct?

Grahame Grieve (Sep 09 2020 at 01:25):

I don't do anything with checkOrderedFunctions

Grahame Grieve (Sep 09 2020 at 01:25):

I do use ordered = true to check whether the outcome is in the right order or not

Grahame Grieve (Sep 09 2020 at 01:26):

I don't do anything about "strict"

Grahame Grieve (Sep 09 2020 at 01:42):

A revisited the tests testDateNotEqualTimezoneOffsetBefore, testDateNotEqualTimezoneOffsetAfter, testDateNotEqualUTC, and agree. I've changed them in the source

Grahame Grieve (Sep 09 2020 at 01:46):

https://github.com/FHIR/fhir-test-cases/commit/2bbdce168f2ff90a1b471fd87d9e1b8ec461a7ef

Ewout Kramer (Sep 09 2020 at 07:46):

I am using 'strict' - these test exercise the section on strict eval (http://hl7.org/fhirpath/N1/#type-safety-and-strict-evaluation) - and since e.g. my engine does not have access to structure definitions I don't run these tests.

Ewout Kramer (Sep 09 2020 at 07:47):

(I have the impression -based on the text in that section- that strict evaluation is optional - so I needed to identify these tests)

Ewout Kramer (Sep 09 2020 at 07:49):

we can clean those things up as far as I am considered by putting them in a named group. I can exclude tests for stuff I don't (yet) implement based on names.

Grahame Grieve (Sep 09 2020 at 09:38):

which ones would we put in a group?

Ewout Kramer (Sep 09 2020 at 09:44):

the ones marked with "strict" now - then the "strict" attribute can be removed.

Grahame Grieve (Sep 09 2020 at 17:26):

given that this would force a re-order and a re-assignment out of functional groups, this sounds like a retro-grade step to me

Ewout Kramer (Sep 10 2020 at 07:25):

Yeah, that would be a consequence indeed.

Bryn Rhodes (Sep 16 2020 at 02:15):

Okay, addressed changes from @Ewout Kramer 's testing with this PR

Paul Lynch (Sep 16 2020 at 13:11):

@Bryn Rhodes Where is the "encode" function (used in those tests) defined?

Bryn Rhodes (Sep 16 2020 at 13:20):

It's in a PR on the FHIRPath repository: http://build.fhir.org/ig/HL7/FHIRPath/branches/feature-27757-additional-string-functions/#additional-string-functions

Paul Lynch (Sep 16 2020 at 13:22):

A new version of the FHIRPath specification is being worked on? When will it be balloted, or did it miss it?

Bryn Rhodes (Sep 16 2020 at 13:23):

Not ready for ballot, no, but there are a lot of trackers that have been submitted, see the current outstanding items in JIRA for the whole list, but many of them still need discussion and resolution.

Bryn Rhodes (Sep 16 2020 at 19:38):

Other than trackers being submitted as issues are found, there hasn't been a lot of activity. ITS is the Work Group that stewards FHIRPath, so discussions will happen there once we get enough dispositions together to support a block vote or discussion on an ITS call. That is on my list, and I'll certainly post here when I make forward progress on it. And of course you (and anyone else) are welcome to comment on the trackers there to help come to resolution on them.

Grahame Grieve (Sep 17 2020 at 04:37):

@Bryn Rhodes I don't understand this:

Bryn says:

Agreed on testIntegerBooleanNotTrue, that should result in an error, since a boolean-valued function is being invoked on a non-boolean-valued argument. Same with testIntegerBooleanNotFalse, so I've added invalid="semantic" to those tests

Bryn makes a PR, that changes the test from

<test name="testIntegerBooleanNotTrue" inputfile="patient-example.xml"><expression>(0).not() = true</expression><output type="boolean">true</output></test>

to

<test name="testIntegerBooleanNotTrue" inputfile="patient-example.xml"><expression>(0).not() = false</expression><output type="boolean">true</output></test>

Grahame Grieve (Sep 17 2020 at 04:40):

I find the basis for this slippery since .not() is not formally defined. In most languages, you can apply not to an integer value as a boolean test

Bryn Rhodes (Sep 17 2020 at 04:41):

Yes, sorry for the confusion, but as Ewout pointed out, if the singleton vale cannot be implicitly converted to a boolean (and an integer requires explicit conversion), then the collection is evaluated as true if it contains a single element, no matter what the element is.

Bryn Rhodes (Sep 17 2020 at 04:42):

The alternative is to say that integers can implicitly convert to booleans

Grahame Grieve (Sep 17 2020 at 04:49):

not necessarily. The alternative would be to say that .Not() can be applied to integers

Grahame Grieve (Sep 17 2020 at 04:50):

I don't feel strongly for that, but there is no definition for .not()

Grahame Grieve (Sep 17 2020 at 04:50):

but there is this:

Grahame Grieve (Sep 17 2020 at 04:51):

IF the collection contains a single node AND the expected input type is Boolean THEN
The collection evaluates to true

Grahame Grieve (Sep 17 2020 at 04:51):

because of this, (0).not() = false

Grahame Grieve (Sep 17 2020 at 05:04):

so I think your commit was right and your comments were not

Grahame Grieve (Sep 17 2020 at 05:06):

but this makes a mess of this test:

Grahame Grieve (Sep 17 2020 at 05:09):

        <test name="from-zilip-2" inputfile="patient-example.xml"><expression invalid="semantic">(true | 'foo').allTrue()</expression><output type="boolean">false</output></test>

Grahame Grieve (Sep 17 2020 at 05:18):

no. not a mess. But definitely beyond the definition, since it doesn't say what to do about items that are not convertible to boolean.

Bryn Rhodes (Sep 17 2020 at 05:22):

Right, for that test it’s an error because foo can’t be converted to boolean. For systems that can check types statically, it’s a compile-time error, and a runtime one for dynamically typed systems.

Grahame Grieve (Sep 17 2020 at 05:24):

I check types statically, but that's still beyond me to be sure that's an error at compile time

Grahame Grieve (Sep 17 2020 at 05:24):

I know at compile time that the collection might contain a string, but not that it is sure to

Bryn Rhodes (Sep 17 2020 at 05:25):

Defining not for integers specifically would be an option, but I will note that .not() is formally defined in the spec: http://hl7.org/fhirpath/#not-boolean

Grahame Grieve (Sep 17 2020 at 05:26):

sigh apparently I was searching for .not() not not()

Bryn Rhodes (Sep 17 2020 at 05:27):

And defining it only for not() would I think lead to some inconsistent behaviors with the other logical operators.

Ewout Kramer (Sep 17 2020 at 08:49):

Bryn Rhodes said:

And defining it only for not() would I think lead to some inconsistent behaviors with the other logical operators.

Right, and that's why not() is slightly inconsistent with other operators working on a focus (like allTrue()): not() explicitly applies the logic of singleton evaluation (http://hl7.org/fhirpath/#singleton-evaluation-of-collections) to the focus - whereas the others do not. Am I right?

Grahame Grieve (Sep 17 2020 at 11:31):

the others apply it to the individuals items in the collection

Paul Lynch (Sep 17 2020 at 15:27):

Grahame Grieve said:

the others apply it to the individuals items in the collection

I am not sure how to interpret that. The "it" seems to refer to http://hl7.org/fhirpath/#singleton-evaluation-of-collections, but that section is explicitly only about functions and operators expecting a singleton item.

Are you saying that allTrue() should convert convertible things in its input collection to Booleans, and non-convertible things to true?

Grahame Grieve (Sep 17 2020 at 19:59):

That tests presently say that allTrue() should convert convertible things in its input collection to Booleans, and blow up on non-convertible things

Last updated: Apr 12 2022 at 19:14 UTC