Perform unit conversion in equality, comparison and arithmetic operators for Quantity (and Ratio) #1578
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
scymtym
changed the title
Contributor
Author
|
@antvaset I merged master into this a few more times but I'm not sure what the next step is. Is there anything I should be doing to make progress with this? |
If the Quantity.equal method indicates that the units of the supplied Quantity instances are not comparable by returning null, try to convert the value of the "right" Quantity to the unit of the "left" Quantity using the UCUM service. And similarly for the Quantity.equivalent method with the difference that the Equivalent evaluator returns false if the units are not comparable. A new method Ratio.fullEquivalent accepts a State and passes it to EquivalentEvaluator.equivalent so that unit conversion can be used.
AddEvaluator and SubtractEvaluator, if necessary, use the UnitConversionHelper to convert the unit of one of their operands. This strategy requires the UcumService to be accessible in the two evaluators which in turn means that any evaluator which performs addition or subtraction now requires the State instance. A few tests which had to be skipped before this change succeed now. However, the tests QtyIvlCollapse_CollapseQuantityUnits[Not]WithinPer now have to be skipped until comparison operators for Quantity consider unit conversion.
* {Less:,LessOrEqual,Greater,GreaterOrEqual}Evaluator use the new
helper function compareQuantities to support comparison of Quantities
with different but comparable units
* CqlList uses the new comparison behavior when sorting, and therefore
needs the current State instance
* Interval.get{Start,End} for the unbounded side of an interval return
a Quantity with the correct unit (the unit of the opposite end point)
* Enable previously skipped tests
Add implementations in createUcumService and DefaultUcumService.
I think this is because Interval with Quantity end points of different units now work but are expected not work. * MultiplyEvaluator and DivideEvaluator, if necessary, use the UcumService to compute the value and unit of the operation result * The various Variance and StdDev evaluators now shared code for the variance computation. The variance computation can produce a quantity with either a squared or a non-squared unit so that the both variances and standard deviations use to correct unit. * Enable previously skipped tests which involve the above evaluators and unit conversion
scymtym
force-pushed
the
quantity-unit-conversion
branch
from
fe0ed6d to
2d88889
Compare
Contributor
Author
|
Updated to apply to the Kotlin conversion of the codebase as discussed in Zulip chat. All tests that I could run locally, including many newly enabled tests, pass for me. The issues mentioned in the original pull request description have been resolved. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
The pull request mostly achieves what is claimed but has draft status for now because of the following issues:
RuntimeTest.intervalOfQuantityWithDifferentUOMfails now because the expectedInvalidIntervalexception is not thrown.I think the test is wrong because both interval endpoints have the unit "mass per volume" (although spelled scaled differently) and should be acceptable for defining an
Interval<Quantity>.In the test
CQLOperationsR4Test.test, the test casetestQuantity/testQuantity4fails.The test case asserts that
4 'g' ~ 4040 'mg'is true.I can only imagine this being true due to some precision rule, but I had the impression that for unit conversion, everything has to be converted to the "more granular" unit which would mean
4000 'mg' ~ 4040 'mg'which has to be false.There are a few TODOs, mostly related to test improvements, that I would address if this pull request is otherwise acceptable.