issue#105 add UCUM support, start with arithemitc function support #106
Conversation
| } | ||
|
|
||
| // ConvertUnit converts a value from one unit to another | ||
| func ConvertUnit(fromVal float64, fromUnit, toUnit string) (float64, error) { |
There was a problem hiding this comment.
What if we changed this API to be ConvertUnit(fromQuantity result.Quantity, newUnit string) (result.Quantity, error)?
This would enable callers to to do something like
foo, err = ConvertUnit(foo, bar.unit)
which i thing would be slightly better for usability.
| @@ -0,0 +1,36 @@ | |||
| // Copyright 2024 Google LLC | |||
There was a problem hiding this comment.
Per the guidance at https://google.github.io/styleguide/go/best-practices.html#util-packages
I would recommend we avoid naming this class helpers, as that doesn't really help the reader know what value this file provides.
I think a better option here would be calling this units.go, the functions this file provides seem to all be related to handling logic related to units. Additionally, doing this would allow us to roll some of the other unit specific logic from ucum.go into this file so ucum.go can provide all the public APIs for this module and units.go can provide logic and constants for units.
WDYT?
| } | ||
|
|
||
| // FixUnit applies both empty and date unit fixes | ||
| func FixUnit(unit string) string { |
There was a problem hiding this comment.
What do you think of using the verb normalize or clean here in stread of fix?
| package ucum | ||
|
|
||
| // FixEmptyUnit handles null/empty units, replacing them with "1" (dimensionless unit) | ||
| func FixEmptyUnit(unit string) string { |
There was a problem hiding this comment.
Should this function as well as FixCQLDateUnit be public, or should the public api of ucum.go just call the fixUnit function at the public boundaries?
| // See the License for the specific language governing permissions and | ||
| // limitations under the License. | ||
|
|
||
| // Package ucum provides UCUM (Unified Code for Units of Measure) support for the CQL engine. |
There was a problem hiding this comment.
Would you mind adding a link to https://unitsofmeasure.org/ucum as well as https://cql.hl7.org/02-authorsguide.html#quantities here? :)
| valid, msg := ucum.CheckUnit(unitStr, true, true) | ||
| if !valid { | ||
| // Just log a warning and continue - don't block parsing | ||
| fmt.Printf("Warning: %s\n", msg) |
|
|
||
| // CQLToUCUMDateUnits maps CQL date units to their UCUM equivalents | ||
| var CQLToUCUMDateUnits = map[string]string{ | ||
| "years": "a_g", |
There was a problem hiding this comment.
I think this is right, but i cant remember / find the right docs on some of the specifics here. Can you add comments on why the _g is added?
https://cql.hl7.org/02-authorsguide.html#quantities seems to suggest that "a" is the equivalent here. But I feel like I've also seen this syntax before.
There was a problem hiding this comment.
_g = gregorian
_j = julian
https://github.com/LHNCBC/ucum-lhc/blob/master/data/ucum.csv#L278
This is the npm package used in the javascript cql engine, which is where I started. Since go does not have a ucum package this file was intended to be a minimal replacement for not having the equivalent package.
If would ask about the ucum support vs fhir support vs the cql spec on unit conversion that I do not have any answers to 😖
There was a problem hiding this comment.
Thanks for the link here! Mind adding that link to the docs at the top of the file? It might help if we find further edge cases in the future. If this is what the Javascript CQL engine is using than I'm more apt to say we can go with this solution for now :)
| conversionFunc := ctx.GetChild(0).(antlr.TerminalNode).GetText() | ||
| expr := v.VisitExpression(ctx.Expression()) | ||
|
|
||
| if conversionFunc == "convert" { |
There was a problem hiding this comment.
consider reversing this case,
if conversionFunc != "covert" { ... }
that way we can unnest most of this function's code.
| } | ||
| } | ||
|
|
||
| func (v *visitor) VisitConversionExpressionTerm(ctx *cql.ConversionExpressionTermContext) model.IExpression { |
There was a problem hiding this comment.
there's a lot of cases in this, can we get some tests that hit this code?
| valid, msg := ucum.CheckUnit(unit, true, true) | ||
| if !valid { | ||
| // Log warning but proceed | ||
| fmt.Printf("Warning: %s\n", msg) |
There was a problem hiding this comment.
According to the spec https://cql.hl7.org/09-b-cqlreference.html#toquantity the result should be null here.
When adding tests please make sure this case is covered.
|
Thanks for taking a crack at this! The UCUM standard is a complicated space, it might take a few rounds of reviews here to get this to a good spot, so this is a great start! |
2 participants
No description provided.