Treat a date-only measurement period end as end-of-day - #372
Open
Taleef7 wants to merge 1 commit into
Open
Conversation
A measurementPeriodEnd given as a plain date ("2019-12-31") was parsed as midnight
at the start of that day, so anything recorded on the final day of the period fell
outside the interval. Nothing errored; the calculation was just slightly wrong.
getCQLIntervalEndpoints now resolves a date-only end to that day's last millisecond.
Calculator has already folded Measure.effectivePeriod.end into the options by the
time it runs, so the caller-supplied and default paths are both covered by the one
change. A value that already carries a time is parsed exactly as before.
This also affects DEFAULT_MEASUREMENT_PERIOD_END, which is itself date-only
('2019-12-31'), so callers relying on the default were losing December 31st too.
Four existing expectations encoded the old reading and are updated. Three are in
DataRequirementHelpers.test.ts, which built the expected interval by parsing the
date-only end as start-of-day; createIntervalFromEndpoints shares this resolver,
and a FHIR Period.end is inclusive of the day it names, so the same reading
applies there. The fourth is the pretty-various-types integration test, whose
measurement period is 2022-01-01 to 2022-12-31 and whose printed interval now
ends at 12/31/2022 11:59:59 PM instead of 12:00:00 AM.
Verified against the published MADiE decks for CMS122 and CMS125: 119/121 before,
121/121 after. Both previously failing cases were CMS125 patients whose only
qualifying mammogram fell on the last day of the measurement period.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes #371.
A
measurementPeriodEndgiven as a plain date ("2019-12-31") is parsed as midnight at the start of that day, so anything recorded on the final day of the period falls outside the interval. Nothing errors — the result is just quietly wrong.I found this building a regression harness around the published MADiE decks for CMS122 and CMS125. With a date-only end it scored 119/121; with this change it scores 121/121. Both failures were CMS125 patients whose only qualifying mammogram fell on the last day of the measurement period, which is part of why it is easy to miss — CMS122's numerator does not tend to sit on the boundary.
Worth noting separately:
DEFAULT_MEASUREMENT_PERIOD_ENDis itself'2019-12-31', so callers relying on the default have been losing December 31st as well.New behavior
A date-only measurement period end now resolves to
23:59:59.999on that day. An end that already carries a time is parsed exactly as before, so anyone passing a full instant sees no change.measurementPeriodStartis untouched — start-of-day is already correct there.This will move results for any measure bundle whose
effectivePeriod.endis date-only, which includes the published eCQM content. The regression script will report differences, and that is the fix working rather than a failure — worth a careful look before merge.Code changes
getCQLIntervalEndpointsis the single place both paths converge: by the time it runs,Calculatorhas already foldedMeasure.effectivePeriod.endinto the options, so one change covers the caller-supplied and default paths alike. That matches the scope I sketched on the issue.The normalization itself is a new
parseTimeStringAsUTCConvertingToEndOfDayinValueSetHelper, alongside the existingparseTimeStringAsUTCConvertingToEndOfYear.Four existing expectations encoded the old reading and are updated here:
test/unit/DataRequirementHelpers.test.ts, which built the expected interval by parsing the date-only end as start-of-day.test/integration/pretty-various-types, whose measurement period is2022-01-01to2022-12-31and whose printed interval now ends at12/31/2022 11:59:59 PMrather than12:00:00 AM.One thing to check me on.
DataRequirementHelpers.createIntervalFromEndpointsshares this resolver, so data requirements inherit the change. I believe that is right — a FHIRPeriod.endis inclusive of the day it names, and it is the same measurement period either way — but it is the one scope decision I would not want to make silently. If you would rather data requirements keep the old reading, say so and I will narrow the change to the calculation path.Testing guidance
npm run checkpasses (477 unit tests, lint, prettier) andnpm run test:integrationpasses (46 tests).New
test/unit/Execution.test.tscovers the behavior directly: a date-only end resolves to end-of-day, a datetime end is untouched, the default resolves to end-of-day, and the start stays at start-of-day. There was no existing unit test file forExecution.ts.To verify end to end, calculate any measure against a patient whose only qualifying event falls on the last day of the period, passing
measurementPeriodEndas a bare date. Before this change the patient misses the population; after it, they are in it.I also ran this against the published CMS122 and CMS125 MADiE test decks through my own harness, swapping only the built library underneath it: stock 1.8.5 scores 119/121, this branch scores 121/121, with no other change on my side.