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.

Sign up for GitHub

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

Jump to bottom

Explore elm types #267

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from
Draft

Explore elm types #267

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
4ea146e
explore types
csenn Mar 14, 2022
d14d548
a few more types
csenn Mar 14, 2022
cbee817
conditional types
csenn Mar 14, 2022
e7c3267
run prettier
csenn Mar 14, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/datatypes/datetime.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -631,7 +631,7 @@ export class DateTime extends AbstractDate {
}

// TODO: Note: using the jsDate type causes issues, fix later
static fromJSDate(date: any, timezoneOffset?: any) {
static fromJSDate(date: any, timezoneOffset?: any): DateTime {
//This is from a JS Date, not a CQL Date
if (date instanceof DateTime) {
return date;
Expand Down
35 changes: 21 additions & 14 deletions src/datatypes/quantity.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,15 +29,15 @@ export class Quantity {
return true;
}

clone() {
clone(): Quantity {
return new Quantity(this.value, this.unit);
}

toString() {
toString(): string {
return `${this.value} '${this.unit}'`;
}

sameOrBefore(other: any) {
sameOrBefore(other: any): null | boolean {
if (other != null && other.isQuantity) {
const otherVal = convertUnit(other.value, other.unit, this.unit);
if (otherVal == null) {
Expand All @@ -46,9 +46,10 @@ export class Quantity {
return this.value <= otherVal;
}
}
return null;
}

sameOrAfter(other: any) {
sameOrAfter(other: any): null | boolean {
if (other != null && other.isQuantity) {
const otherVal = convertUnit(other.value, other.unit, this.unit);
if (otherVal == null) {
Expand All @@ -57,9 +58,10 @@ export class Quantity {
return this.value >= otherVal;
}
}
return null;
}

after(other: any) {
after(other: any): null | boolean {
if (other != null && other.isQuantity) {
const otherVal = convertUnit(other.value, other.unit, this.unit);
if (otherVal == null) {
Expand All @@ -68,9 +70,10 @@ export class Quantity {
return this.value > otherVal;
}
}
return null;
}

before(other: any) {
before(other: any): null | boolean {
if (other != null && other.isQuantity) {
const otherVal = convertUnit(other.value, other.unit, this.unit);
if (otherVal == null) {
Expand All @@ -79,9 +82,10 @@ export class Quantity {
return this.value < otherVal;
}
}
return null;
}

equals(other: any) {
equals(other: any): null | boolean {
if (other != null && other.isQuantity) {
if ((!this.unit && other.unit) || (this.unit && !other.unit)) {
return false;
Expand All @@ -96,15 +100,16 @@ export class Quantity {
}
}
}
return null;
}

convertUnit(toUnit: any) {
convertUnit(toUnit: any): Quantity {
const value = convertUnit(this.value, this.unit, toUnit);
// Need to pass through constructor again to catch invalid units
return new Quantity(value, toUnit);
}

dividedBy(other: any) {
dividedBy(other: any): Quantity | null {
if (other == null || other === 0 || other.value === 0) {
return null;
} else if (!other.isQuantity) {
Expand All @@ -128,7 +133,7 @@ export class Quantity {
return new Quantity(decimalAdjust('round', resultValue, -8), resultUnit);
}

multiplyBy(other: any) {
multiplyBy(other: any): Quantity | null {
if (other == null) {
return null;
} else if (!other.isQuantity) {
Expand All @@ -153,7 +158,7 @@ export class Quantity {
}
}

export function parseQuantity(str: string) {
export function parseQuantity(str: string): Quantity | null {
const components = /([+|-]?\d+\.?\d*)\s*('(.+)')?/.exec(str);
if (components != null && components[1] != null) {
const value = parseFloat(components[1]);
Expand Down Expand Up @@ -206,16 +211,18 @@ export function doSubtraction(a: any, b: any) {
return doScaledAddition(a, b, -1);
}

export function doDivision(a: any, b: any) {
export function doDivision(a: Quantity | null, b: Quantity | null): Quantity | null {
if (a != null && a.isQuantity) {
return a.dividedBy(b);
}
return null;
}

export function doMultiplication(a: any, b: any) {
export function doMultiplication(a: Quantity | null, b: Quantity | null): Quantity | null {
if (a != null && a.isQuantity) {
return a.multiplyBy(b);
} else {
} else if (b != null && b.isQuantity) {
return b.multiplyBy(a);
}
return null;
}
55 changes: 32 additions & 23 deletions src/elm/arithmetic.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,9 +11,10 @@ import { Uncertainty } from '../datatypes/uncertainty';
import { Context } from '../runtime/context';
import { build } from './builder';
import { DateTime } from '../datatypes/datetime';
import ELM from '../types/elm';

export class Add extends Expression {
constructor(json: any) {
constructor(json: ELM.Add) {
super(json);
}

Expand Down Expand Up @@ -56,7 +57,7 @@ export class Add extends Expression {
}

export class Subtract extends Expression {
constructor(json: any) {
constructor(json: ELM.Subtract) {
super(json);
}

Expand Down Expand Up @@ -94,7 +95,7 @@ export class Subtract extends Expression {
}

export class Multiply extends Expression {
constructor(json: any) {
constructor(json: ELM.Multiply) {
super(json);
}

Expand Down Expand Up @@ -132,7 +133,7 @@ export class Multiply extends Expression {
}

export class Divide extends Expression {
constructor(json: any) {
constructor(json: ELM.Divide) {
super(json);
}

Expand Down Expand Up @@ -172,7 +173,7 @@ export class Divide extends Expression {
}

export class TruncatedDivide extends Expression {
constructor(json: any) {
constructor(json: ELM.TruncatedDivide) {
super(json);
}

Expand All @@ -193,7 +194,7 @@ export class TruncatedDivide extends Expression {
}

export class Modulo extends Expression {
constructor(json: any) {
constructor(json: ELM.Modulo) {
super(json);
}

Expand All @@ -210,7 +211,7 @@ export class Modulo extends Expression {
}

export class Ceiling extends Expression {
constructor(json: any) {
constructor(json: ELM.Ceiling) {
super(json);
}

Expand All @@ -225,7 +226,7 @@ export class Ceiling extends Expression {
}

export class Floor extends Expression {
constructor(json: any) {
constructor(json: ELM.Floor) {
super(json);
}

Expand All @@ -240,7 +241,7 @@ export class Floor extends Expression {
}

export class Truncate extends Expression {
constructor(json: any) {
constructor(json: ELM.Truncate) {
super(json);
}

Expand All @@ -254,7 +255,7 @@ export class Truncate extends Expression {
}
}
export class Abs extends Expression {
constructor(json: any) {
constructor(json: ELM.Abs) {
super(json);
}

Expand All @@ -271,7 +272,7 @@ export class Abs extends Expression {
}

export class Negate extends Expression {
constructor(json: any) {
constructor(json: ELM.Negate) {
super(json);
}

Expand All @@ -290,7 +291,7 @@ export class Negate extends Expression {
export class Round extends Expression {
precision: any;

constructor(json: any) {
constructor(json: ELM.Round) {
super(json);
this.precision = build(json.precision);
}
Expand All @@ -307,7 +308,7 @@ export class Round extends Expression {
}

export class Ln extends Expression {
constructor(json: any) {
constructor(json: ELM.Ln) {
super(json);
}

Expand All @@ -324,7 +325,7 @@ export class Ln extends Expression {
}

export class Exp extends Expression {
constructor(json: any) {
constructor(json: ELM.Exp) {
super(json);
}

Expand All @@ -344,7 +345,7 @@ export class Exp extends Expression {
}

export class Log extends Expression {
constructor(json: any) {
constructor(json: ELM.Log) {
super(json);
}

Expand All @@ -361,7 +362,7 @@ export class Log extends Expression {
}

export class Power extends Expression {
constructor(json: any) {
constructor(json: ELM.Power) {
super(json);
}

Expand All @@ -381,7 +382,7 @@ export class Power extends Expression {
}

export class MinValue extends Expression {
static readonly MIN_VALUES = {
static MIN_VALUES = {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason to take out the readonly qualifier here? The value of MIN_VALUES never gets re-assigned. I saw that MAX_VALUES below still has readonly, so figured I'd ask about the change

Copy link
Author

Choose a reason for hiding this comment

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

Nope, just added readonly back!

Copy link
Member

Choose a reason for hiding this comment

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

@csenn - I'm taking a look at this PR again, as it would be great to get it resolved. I noticed that you said you added the readonly back on line 385, but when I look at the current code on this branch, I don't see it. Perhaps you have not pushed it back up?

'{urn:hl7-org:elm-types:r1}Integer': MathUtil.MIN_INT_VALUE,
'{urn:hl7-org:elm-types:r1}Decimal': MathUtil.MIN_FLOAT_VALUE,
'{urn:hl7-org:elm-types:r1}DateTime': MathUtil.MIN_DATETIME_VALUE,
Expand All @@ -391,9 +392,13 @@ export class MinValue extends Expression {

valueType: keyof typeof MinValue.MIN_VALUES;

constructor(json: any) {
constructor(json: ELM.MinValue) {
super(json);
this.valueType = json.valueType;
if (json.valueType in MinValue.MIN_VALUES) {
this.valueType = json.valueType as keyof typeof MinValue.MIN_VALUES;
Copy link
Contributor

Choose a reason for hiding this comment

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

Small (opinionated) comment. Now that we use keyof typeof MinValue.MIN_VALUES in more than one place, maybe we can bring it out to its own type outside of the class? E.g.:

type MinValueType = keyof typeof MinValue.MIN_VALUES;

Then we can just re-use this type where applicable. Same applies to max values below

Copy link
Author

Choose a reason for hiding this comment

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

It also does make sense to pull MIN_VALUE out, and maybe even use an ENUM instead of a dict. But it was a little weird to even have to use as keyof typeof MinValue.MIN_VALUES... I thought Typescript would be able to infer that since json.valueType in MinValue.MIN_VALUES must be true

} else {
throw new Error(`Not expecting MIN_VALUE: ${json.valueType})`);
Copy link
Contributor

Choose a reason for hiding this comment

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

This error check breaks the arithmetic tests, but for a very logical reason lol. There is a test case that asserts that any incorrect MinValue provided should throw an error during execution. The generated ELM for this test case is here.

Since the constructor now throws an error if provided a valueType that we don't recognize.

I'm going to defer to @cmoesel here on how exactly to proceed. I think I might lean towards not throwing an error in a constructor, but I see the merit in the code that you've added.

Copy link
Author

Choose a reason for hiding this comment

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

Yeah def agree with you, both ways have merit. Here is a StackOverflow discussing. I semi-lean towards validation in the constructor as long as that runs over the whole ELM tree as soon as the ELM wrapper is instantiated, just so that it doesn't become possible for execution to 'skip' certain parts of the ELM tree depending on the input patient data (only exec MinValue node if Patient has Condition Y) which could then allow bugs to escape tests more easily. Not sure if that's applicable though?

Either way, MIN_VALUES could be an ENUM but SHOULD NOT be added to the ELM generated from the XML because the mappings to MathUtil.MIN_INT_VALUE seem to be numbers that are only specific to the JS implementation.

Copy link
Member

Choose a reason for hiding this comment

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

I can see both arguments -- including the desire to fail immediately upon loading the ELM (rather than on execution, just in case execution never occurs). That said the spec does say this:

For any other type, attempting to invoke minimum results in an error.

One could argue that "attempting to invoke" implies an error on execution, not on loading. (As far as I can tell, this is how the Java cql-engine implementation behaves as well).

}
}

exec(ctx: Context) {
Expand Down Expand Up @@ -422,9 +427,13 @@ export class MaxValue extends Expression {

valueType: keyof typeof MaxValue.MAX_VALUES;

constructor(json: any) {
constructor(json: ELM.MaxValue) {
super(json);
this.valueType = json.valueType;
if (json.valueType in MaxValue.MAX_VALUES) {
this.valueType = json.valueType as keyof typeof MaxValue.MAX_VALUES;
} else {
throw new Error(`Not expecting Max_VALUE: ${json.valueType})`);
Copy link
Contributor

Choose a reason for hiding this comment

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

Same deal here as my comment about errors for MinValue.

}
}

exec(ctx: Context) {
Expand All @@ -443,7 +452,7 @@ export class MaxValue extends Expression {
}

export class Successor extends Expression {
constructor(json: any) {
constructor(json: ELM.Successor) {
super(json);
}

Expand Down Expand Up @@ -472,7 +481,7 @@ export class Successor extends Expression {
}

export class Predecessor extends Expression {
constructor(json: any) {
constructor(json: ELM.Predecessor) {
super(json);
}

Expand Down
Loading