1024 fix unit normalization for quantity units such as ucum and calendar date units #1037
Conversation
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
…ions retrieve Display while generating FHIR components
…ract operation should not be allowed, as it could not add x day to 9999/12/31 and subtract x day from 01/01/01
…alendar. Ucum follows fixed quantity calculations. Fixed logic to allow both units and calculate accordingly. Updated/Added unit tests.
…endar-based-CqlQuantity
…endar-based-CqlQuantity
…endar-based-CqlQuantity
…endar-based-CqlQuantity
…endar-based-CqlQuantity
Co-authored-by: Copilot <[email protected]>
…lQuantity' of https://github.com/ncqa-org/firely-cql-sdk-v2.0-hedis-test into Unit-normalization-fixes-for-UCUM-and-Calendar-based-CqlQuantity
Refactored the `ToDateTimePrecision` method to use a modern C# switch expression, improving readability and reducing boilerplate. Combined multiple cases into single lines for compactness. Updated the copyright header comment to remove unnecessary characters and ensure proper formatting.
Updated `CqlDate_Subtract_Months_From_Year` test to reflect new expected behavior when subtracting months. Simplified `TerminalParsers.cs` by removing the `translateUnit` function and directly using `unitText`. Improved slice-related test cases to handle edge cases and align with CQL specifications. Removed redundant empty lines and comments across files for better readability and consistency.
Simplified `Subtract` methods by delegating to `Add` with negated quantities, reducing duplication. Added `+` and `-` operator overloads for `CqlDate`, `CqlDateTime`, and `CqlTime` to enable intuitive arithmetic operations. Enhanced `CqlQuantity` with negation support via `Negate` method and `-` operator. Improved precision handling in `CqlTime` with explicit handling for `DateTimePrecision.Unknown`. Fixed arithmetic logic for year (`"a"`) and month (`"mo"`) units to account for value signs. Overrode `Equals` and `GetHashCode` for proper equality and hashing in `CqlDate`, `CqlDateTime`, and `CqlTime`. Refactored `CqlTime` constructor for compactness and added `MinValue`/`MaxValue` static properties. Improved boundary calculation methods for readability. Updated test case in `PrimitiveTests.cs` to reflect corrected subtraction logic. Performed general code cleanup to improve readability and maintainability.
Refactored `PrimitiveTests.cs` to improve organization and modularity: - Renamed `PrimitiveTests` to `CqlDateTests`. - Introduced `CqlDateTimeTests` for `CqlDateTime`-related tests. - Moved `GetNewContext` method to `CqlDateTests` and `CqlDateTimeTests`. - Added new tests: `CqlDateTime_Add_Year_By_Units` and `CqlDateTime_Subtract_Day_and_Days`. - Reintroduced `PrimitiveTests` with a placeholder method and new test. Cleaned up `using` directives: - Removed unused namespaces. - Added necessary namespaces like `Hl7.Cql.Iso8601` and `System.Linq.Expressions`. Refactored `CqlDateTime_Subtract_Day_and_Days` for readability: - Reformatted LINQ query for method retrieval. - Introduced `rc` variable for context access. Updated subproject references: - Marked `Firely.Cql.Sdk.Integration.Runner` and `Ncqa.DQIC` as `-dirty`. Introduced new files: - `CqlDateTests.cs` and `CqlDateTimeTests.cs` to house refactored test classes.
The submodules `Firely.Cql.Sdk.Integration.Runner` and `Ncqa.DQIC` were updated. Both submodules are now in a "dirty" state, indicating the presence of uncommitted local changes.
Enabled `#nullable` annotations to improve null safety and reorganized `using` directives for clarity. Added a new `CqlQuantityTests` class with unit tests to validate `CqlQuantity` behavior, including negation operations and handling of `null` values. Made minor formatting adjustments for consistency and readability.
Replaced abbreviated unit strings with full names (e.g., "a" to "year", "mo" to "month") in the `Hl7.Cql.CqlToElm.Test` namespace. Applied pluralization where appropriate (e.g., "years", "months") and ensured consistency across all test methods.
Improved the handling of unit text in the `TerminalParsers.cs` file within the `Hl7.Cql.CqlToElm.Visitors` namespace. Added a comment noting that unit range validation is not yet implemented and updated the method to return a tuple containing the parsed decimal value and the unit text. This change enhances code clarity and ensures proper handling of unit text.
Replaced hardcoded unit strings with `UCUMUnits` constants across the codebase to improve readability, maintainability, and consistency. Added new constants for various UCUM units, including `Year`, `Month`, `Day`, and others. Introduced `DaysPerYearDouble` and `DaysPerMonthDouble` for better precision in date calculations. Updated `CqlDateTime.cs`, `CqlDateTimeMath.cs`, and `CqlTime.cs` to use these constants, consolidating logic and improving error handling. Added `NotImplementedException` placeholders for certain cases requiring further implementation. Removed the unused `NormalizeTo` method. Enhanced inline documentation and standardized unit handling to reduce redundancy and potential errors.
There was a problem hiding this comment.
Pull Request Overview
This PR implements unit normalization for calendar date and UCUM units, fixing issue #1024. The changes standardize unit handling by moving from UCUM abbreviations to full English unit names (e.g., "a" → "year", "mo" → "month") while maintaining compatibility with both singular and plural forms.
Key Changes
- Replaced UCUM unit abbreviations with full English unit names throughout the codebase
- Updated unit conversion logic to handle both singular/plural forms and support mixed unit arithmetic
- Modified the CQL-to-ELM parser to preserve original unit text instead of translating to UCUM codes
Reviewed Changes
Copilot reviewed 30 out of 30 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| Cql/Iso8601/DateTimePrecision.cs | Converted switch-based unit parsing to modern pattern matching with full unit names |
| Cql/Cql.Runtime/Operators/CqlOperators.*.cs | Updated all operator classes to use full unit names instead of UCUM codes |
| Cql/Cql.Runtime/Conversion/UnitConverter.cs | Removed UCUM unit translation logic |
| Cql/Cql.Runtime/Operators/CqlOperators.ArithmeticOperators.cs | Added unit normalization for mixed arithmetic operations |
| Cql/Cql.Abstractions/Primitives/CqlTime.cs | Modernized unit handling and added operator overloads |
| Cql/Cql.Abstractions/Primitives/CqlQuantity.cs | Added negation operators and removed UCUM translation |
| Cql/Cql.Abstractions/Primitives/CqlDateTime.cs | Streamlined unit handling with pattern matching |
| Cql/Cql.Abstractions/Primitives/CqlDate.cs | Updated to use full unit names and added operators |
| Cql/Cql.Abstractions/Abstractions/Units.cs | Replaced UCUM mapping with DatePrecision to CQL units mapping |
| Cql/Cql.CqlToElm/Visitors/TerminalParsers.cs | Removed unit translation, preserving original text |
| Test files | Updated test expectations to reflect new unit naming |
…units-such-as-ucum-and-calendar-date-units
…ucum-and-calendar-date-units' of https://github.com/FirelyTeam/firely-cql-sdk into 1024-fix-unit-normalization-for-quantity-units-such-as-ucum-and-calendar-date-units
alexzautke
approved these changes
Sep 29, 2025
baseTwo
deleted the
1024-fix-unit-normalization-for-quantity-units-such-as-ucum-and-calendar-date-units
branch
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.
This is a continuation of PR #1025 which included changes from an external NCQA repo.
This PR implements unit normalization for calendar date and UCUM units, fixing issue #1024. The changes standardize unit handling by moving from UCUM abbreviations to full English unit names (e.g., "a" → "year", "mo" → "month") while maintaining compatibility with both singular and plural forms.
Key Changes
Submodule PR's (please approve and merge)
Fixes #1024