Skip to content

Ucum normalized#97

Closed
bryantaustin13 wants to merge 5 commits into
cqframework:mainfrom
bryantaustin13:ucumNormalized
Closed

Ucum normalized#97
bryantaustin13 wants to merge 5 commits into
cqframework:mainfrom
bryantaustin13:ucumNormalized

Conversation

@bryantaustin13

@bryantaustin13 bryantaustin13 commented Apr 28, 2026

Copy link
Copy Markdown
Contributor

This branch was built from #96. That one needs to be merged first.

Running the below test fails because the engine returns { "value": 0.0002, "unit": "m2" } which is true and matches the expected output, but not in a normalized way. This fix normalizes so the test passes.

<?xml version="1.0" encoding="utf-8"?>
<tests xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xmlns="http://hl7.org/fhirpath/tests" xsi:schemaLocation="http://hl7.org/fhirpath/tests ../../testSchema/testSchema.xsd"
	   name="CqlArithmeticFunctionsTest">
	<group name="SingleTest" version="1.0">
		<test name="Multiply1CMBy2CM" version="1.0">
			<capability code="ucum-unit-conversion-support" />
			<expression>1.0 'cm' * 2.0 'cm'</expression>
			<output>2.0'cm2'</output>
			<!-- TODO: make the units multiply -->
		</test>
	</group>
</tests>

@brynrhodes brynrhodes left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The runner shouldn't need to be doing unit normalization, the engines should be predictably producing UCUM values based on the semantics for quantity arithmetic.


if (typeof expected === 'number') {
return Math.abs(actual - expected) < 0.00000001;
return Math.abs(Number(actual) - expected) < 0.00000001;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The runner shouldn't be casting here, the implementation should be returning the correct type of value, yes?

@bryantaustin13

Copy link
Copy Markdown
Contributor Author

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants