-
Notifications
You must be signed in to change notification settings - Fork 38
Improved support for CQL Decimal #376
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
base: master
Are you sure you want to change the base?
Changes from all commits
939349e
cc050e9
2e8cc8a
e94e989
abd41e3
4d836af
b6f4638
af6bee9
e18f9eb
f6731b9
976ed9a
6426710
65203ef
a0b4f1d
2e9115d
965111f
937b982
f26d5fb
f913c70
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,223 @@ | ||
| import { Decimal as DecimalJS } from 'decimal.js'; | ||
|
|
||
| // Use a clone rather than DecimalJS.set because decimal.js configuration is otherwise global. | ||
| // This keeps our settings from changing the behavior of other decimal.js instances in | ||
| // the same process. | ||
| // Precision is significant figures (not decimal places); | ||
| // CQL's maximum Decimal value has 28 significant figures, 30 is just a cleaner number. | ||
| const CQLDecimalJS = DecimalJS.clone({ precision: 30 }); | ||
|
|
||
| export type DecimalInput = Decimal | string | number | bigint; | ||
|
|
||
| export type DecimalRoundingMode = DecimalJS.Rounding; | ||
|
|
||
| const MIN_PRECISION_VALUE = CQLDecimalJS.pow(10, -8); | ||
|
|
||
| const CQL_IMPLICIT_SCALE = 8; | ||
| const CQL_IMPLICIT_ROUNDING = CQLDecimalJS.ROUND_HALF_UP; | ||
|
|
||
| export class Decimal { | ||
| private value: DecimalJS; | ||
|
|
||
| private constructor(value: string | number | bigint | DecimalJS) { | ||
| this.value = new CQLDecimalJS(value); | ||
| if (!this.value.isFinite()) { | ||
| throw new Error('Cannot create a decimal with a non-finite value'); | ||
| } | ||
| } | ||
|
|
||
| static from(value: DecimalInput) { | ||
| if (value instanceof Decimal) { | ||
| return value; | ||
| } | ||
|
|
||
| return new Decimal(value); | ||
| } | ||
|
|
||
| get isDecimal() { | ||
| return true; | ||
| } | ||
|
|
||
| normalized() { | ||
| if (this.value.decimalPlaces() <= CQL_IMPLICIT_SCALE) { | ||
| return this; | ||
| } | ||
| return this.setScale(CQL_IMPLICIT_SCALE, CQL_IMPLICIT_ROUNDING); | ||
| } | ||
|
|
||
| // Helper function to reduce repeated boilerplate. | ||
| // Apply the given function with the given operand, and wrap the result in a Decimal. | ||
| private applyWrapper(operation: (value: any) => DecimalJS, other: DecimalInput): Decimal { | ||
| const operand = other instanceof Decimal ? other.value : other; | ||
|
|
||
| return new Decimal(operation.call(this.value, operand)); | ||
| } | ||
|
|
||
| add(other: DecimalInput): Decimal { | ||
| return this.applyWrapper(this.value.add, other); | ||
| } | ||
|
|
||
| subtract(other: DecimalInput): Decimal { | ||
| return this.applyWrapper(this.value.minus, other); | ||
| } | ||
|
|
||
| multiplyBy(other: DecimalInput): Decimal { | ||
| return this.applyWrapper(this.value.times, other); | ||
| } | ||
|
|
||
| divideBy(other: DecimalInput): Decimal { | ||
| if (Decimal.from(other).equals(0)) { | ||
| throw new RangeError('Cannot divide a decimal by zero'); | ||
| } | ||
| return this.applyWrapper(this.value.dividedBy, other); | ||
| } | ||
|
|
||
| modulo(other: DecimalInput) { | ||
| if (Decimal.from(other).equals(0)) { | ||
| throw new RangeError('Cannot calculate decimal modulo by zero'); | ||
| } | ||
| return this.applyWrapper(this.value.mod, other); | ||
| } | ||
|
|
||
| compareTo(other: DecimalInput) { | ||
| if (other instanceof Decimal) { | ||
| return this.value.comparedTo(other.value); | ||
| } | ||
| return this.value.comparedTo(other); | ||
| } | ||
|
|
||
| greaterThan(other: DecimalInput) { | ||
| return this.compareTo(other) > 0; | ||
| } | ||
|
|
||
| greaterThanOrEquals(other: DecimalInput) { | ||
| return this.compareTo(other) >= 0; | ||
| } | ||
|
|
||
| lessThan(other: DecimalInput) { | ||
| return this.compareTo(other) < 0; | ||
| } | ||
|
|
||
| lessThanOrEquals(other: DecimalInput) { | ||
| return this.compareTo(other) <= 0; | ||
| } | ||
|
|
||
| equals(other: DecimalInput) { | ||
| return this.compareTo(other) === 0; | ||
| } | ||
|
|
||
| successor() { | ||
| // TODO: successor should be based on current precision | ||
| // For Decimal, successor is equivalent to adding 1 * the precision of the argument. | ||
| return new Decimal(this.value.add(MIN_PRECISION_VALUE)); | ||
| } | ||
|
|
||
| predecessor() { | ||
| // TODO: predecessor should be based on current precision | ||
| // For Decimal, predecessor is equivalent to subtracting 1 * the precision of the argument. | ||
| return new Decimal(this.value.minus(MIN_PRECISION_VALUE)); | ||
| } | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Supporting precision (and operations that depend on it) is important. It's one of the reasons I wanted to move away from Number. We'll have to figure out a way to support this. I was thinking that we might be able to support it if we stored
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. From offline discussion: I'll implement precision in this PR. Preserving the precision of a literal is straightforward, but the spec doesn't define how to handle precision across arithmetic operators. Eg, what should |
||
|
|
||
| negate() { | ||
| return new Decimal(this.value.neg()); | ||
| } | ||
|
|
||
| abs() { | ||
| return new Decimal(this.value.abs()); | ||
| } | ||
|
|
||
| truncate(): number { | ||
| return this.value.truncated().toNumber(); | ||
| } | ||
|
|
||
| truncated(): Decimal { | ||
| return new Decimal(this.value.truncated()); | ||
| } | ||
|
|
||
| ceil(): number { | ||
| return this.value.ceil().toNumber(); | ||
| } | ||
|
|
||
| floor(): number { | ||
| return this.value.floor().toNumber(); | ||
| } | ||
|
|
||
| isInteger() { | ||
| return this.value.isInteger(); | ||
| } | ||
|
|
||
| power(exponent: DecimalInput) { | ||
| return this.applyWrapper(this.value.toPower, exponent); | ||
| } | ||
|
|
||
| sqrt() { | ||
| return new Decimal(this.value.sqrt()); | ||
| } | ||
|
|
||
| ln() { | ||
| return new Decimal(this.value.ln()); | ||
| } | ||
|
|
||
| exp() { | ||
| return new Decimal(this.value.exp()); | ||
| } | ||
|
|
||
| log(base: DecimalInput) { | ||
| return this.applyWrapper(this.value.log, base); | ||
| } | ||
|
|
||
| round(scale: number) { | ||
| // notes on rounding modes | ||
| // ROUND_HALF_UP "Rounds towards nearest neighbour. If equidistant, rounds away from zero" | ||
| // rounds 0.5 -> 1.0, -0.5 -> -1.0 | ||
| // ROUND_HALF_CEIL "Rounds towards nearest neighbour. If equidistant, rounds towards Infinity" | ||
| // rounds 0.5 -> 1.0, -0.5 -> 0.0 | ||
| // https://mikemcl.github.io/decimal.js/#modes | ||
| return this.setScale(scale, CQLDecimalJS.ROUND_HALF_CEIL); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. CQL 2.0 clarified rounding semantics in response to FHIR-45987:
I'm not sure if that's really a "traditional round", but the example indicates that -0.5 rounds to -1, so it seems like we really ought to be using
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. From offline discussion: this was wrong based on trying to pass some cql-tests. Those have been updated upstream so I'll pull the latest cql-tests and update elsewhere as appropriate |
||
| } | ||
|
|
||
| setScale(scale: number, roundingMode: DecimalRoundingMode = CQLDecimalJS.ROUND_DOWN) { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since CQL Round supports passing in a precision, and since it specifies rounding away from zero, I think it probably makes sense to use the same rounding algorithm when reducing scale generally. That is unless you saw something else in the spec that would indicate otherwise (which is totally possible). |
||
| if (!Number.isInteger(scale) || scale < 0) { | ||
| throw new RangeError('Decimal scale must be a non-negative integer'); | ||
| } | ||
|
|
||
| return new Decimal(this.value.toDecimalPlaces(scale, roundingMode)); | ||
| } | ||
|
|
||
| toInteger() { | ||
| // note that this is permissive and converts non-integral values | ||
| return this.truncate(); | ||
|
Comment on lines
+188
to
+189
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why are we permissive on this? And why truncate (vs. round)? Wouldn't we want 1.99999999 to go to 2)?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. From offline discussion: will remove this function and |
||
| } | ||
|
|
||
| toNumber() { | ||
| return this.value.toNumber(); | ||
| } | ||
|
|
||
| toLong() { | ||
| // note that this is permissive and converts non-integral values | ||
| return BigInt(this.value.truncated().toString()); | ||
|
Comment on lines
+197
to
+198
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same question as for
|
||
| } | ||
|
|
||
| toString() { | ||
| // decimal.js toString can return exponential notation, | ||
| // toFixed always returns normal notation | ||
| // CQL spec expects format (-)?#0.0# | ||
| // https://cql.hl7.org/R2/09-b-cqlreference.html#tostring | ||
| // meaning, optional minus sign, at least one digit, decimal point, at least one digit | ||
| // (# means any number of digits, including none; 0 means a digit must appear) | ||
| // a regex for this is -?\d+\.\d+ | ||
| // so Decimal.from(1).toString() --> "1.0" | ||
| const places = Math.max(1, this.value.decimalPlaces()); | ||
| return this.value.toFixed(places); | ||
| } | ||
|
|
||
| toJSON() { | ||
| return this.toString(); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe we should talk about what makes sense here. The FHIR spec serializes |
||
| } | ||
| } | ||
|
|
||
| export const MAX_DECIMAL_STRING = '99999999999999999999.99999999'; | ||
| export const MIN_DECIMAL_STRING = '-99999999999999999999.99999999'; | ||
|
|
||
| export const MAX_DECIMAL_VALUE = Decimal.from(MAX_DECIMAL_STRING); | ||
| export const MIN_DECIMAL_VALUE = Decimal.from(MIN_DECIMAL_STRING); | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is partially legacy behavior, but do you know why we treat
undefinedandnulldifferently here?