Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Investigate the type of ElementModel.Quantity.Value #2791

Open
Kasdejong opened this issue May 24, 2024 · 1 comment
Open

Investigate the type of ElementModel.Quantity.Value #2791

Kasdejong opened this issue May 24, 2024 · 1 comment
Labels
breaking change This issue/commit causes a breaking change, and requires a major version upgrade

Comments

@Kasdejong
Copy link
Contributor

Describe the bug
Our ElementModel.Quantity datatype has a value field of type System.Decimal. There are two issues with this:

  • Retaining precision of decimals is not guaranteed, especially after performing operations on them (since the same numeric value can have multiple representations). We rely on the .NET framework to correctly guess the precision of our values, which they do not guarantee. This is important for comparisons (if one value falls within the error margin of another) and for calculations (the result of a calculation should be as precise as the least precise of its two operands)
  • While the decimal datatype accepts most common values for quantities, the range of possible values specified in the FHIR specification (http://hl7.org/fhir/datatypes.html#decimal) spans a larger range of possible values than the native .NET decimal type.

Now that our metric service interface works with strings instead of decimals, we could either:

  • Add a backing field of type string to the Quantity datatype (not necessarily breaking, but we should have a way of invalidating and bypassing the original decimal if the value is out of range, which would result in an OverflowException previously)
  • Make the Quantity.Value field a string. This is definitely breaking, but we can look into it for SDK v6.0. This would only affect directly setting this field, as we can have an overloaded constructor for decimals which should work with the correct precision in the current .NET framework (again, no guarantees given) as long as we do not perform operations on said decimal before serializing it into a string.
@Kasdejong Kasdejong added bug breaking change This issue/commit causes a breaking change, and requires a major version upgrade labels May 24, 2024
@ewoutkramer
Copy link
Member

We use decimal in more places:

  • Element.Quantity
  • FHIR.Decimal
  • ITypedElement.Value (and so, internally everywhere in the validator, fhirpath etc)

So, the potential "problem" is bigger (and I knew it was not 100% to the FHIR spec), but anyone working with decimals in .NET will be using, well, decimal. So, that's why I chose to use the normal type. How big is this problem? Is this relevant for, say, lab values? I really don't know.

In any case, I don't mind making FHIR.Decimal use string as a backing value (it aligns well with #2781 that I already proposed for 6.0 I think). I don't know that we can do the same for Element.Quantity (although it also has a concept of raw text).

In any case, the logic in our SDK working with these types are using decimal internally, so the problem remains (if it is there).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change This issue/commit causes a breaking change, and requires a major version upgrade
Projects
None yet
Development

No branches or pull requests

3 participants