Added Support for the ExpandValueset operator #332

sorliog · 2025-02-13T22:37:15Z

Description

Added support for the ExpandValueset operator by adding the expand method in the Valueset class. The expand method is invoked in ExpandValueSet.exec() expression.

Submitter:

This pull request describes why these changes were made
Code diff has been done and been reviewed (it does not contain: additional white space, not applicable code changes, debug statements, etc.)
Tests are included and test edge cases
Tests have been run locally and pass
Code coverage has not gone down and all code touched or added is covered.
Code passes lint and prettier (hint: use npm run test:plus to run tests, lint, and prettier)
All dependent libraries are appropriately updated or have a corresponding PR related to this change
cql4browsers.js built with npm run build:browserify if source changed.

Reviewer:

Name:

Code is maintainable and reusable, reuses existing code and infrastructure where appropriate, and accomplishes the task’s purpose
The tests appropriately test the new code, including edge cases
You have tried to break the code

…er, AdvancedCodeService and ExpandValueSet expression

…pand method to valueset for the ExpandValueset operator

codecov-commenter · 2025-02-13T22:38:24Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 87.11%. Comparing base (2cd244c) to head (5ecd084).

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #332      +/-   ##
==========================================
+ Coverage   87.04%   87.11%   +0.07%     
==========================================
  Files          52       52              
  Lines        4517     4534      +17     
  Branches     1273     1277       +4     
==========================================
+ Hits         3932     3950      +18     
  Misses        377      377              
+ Partials      208      207       -1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

cmoesel

Thank you for this contribution, @sorliog! This looks pretty good to me. I left a few comments regarding small things to change -- but all the important parts seems to be working well!

We like to have more than one review of each PR, so I'm also going to ask another colleague to review this.

Thanks again! We love receiving contributions!

cmoesel · 2025-02-14T16:23:05Z

src/cql-code-service.ts

+// class AdvancedCodeService implements AdvancedTerminologyProvider {
+//   valueSets: ValueSetObject;
+//
+//   constructor(valueSetsJson: ValueSetDictionary = {}) {
+//     this.valueSets = {};
+//     for (const oid in valueSetsJson) {
+//       this.valueSets[oid] = {};
+//       for (const version in valueSetsJson[oid]) {
+//         const codes = valueSetsJson[oid][version].map(
+//           (code: any) => new Code(code.code, code.system, code.version)
+//         );
+//         this.valueSets[oid][version] = new ValueSet(oid, version, codes);
+//       }
+//     }
+//   }
+//
+//   findValueSetsByOid(oid: string): ValueSet[] {
+//     return this.valueSets[oid] ? Object.values(this.valueSets[oid]) : [];
+//   }
+//
+//   findValueSet(oid: string, version?: string): ValueSet | null {
+//     if (version != null) {
+//       return this.valueSets[oid] != null ? this.valueSets[oid][version] : null;
+//     } else {
+//       const results = this.findValueSetsByOid(oid);
+//       if (results.length === 0) {
+//         return null;
+//       } else {
+//         return results.reduce((a: any, b: any) => {
+//           if (a.version > b.version) {
+//             return a;
+//           } else {
+//             return b;
+//           }
+//         });
+//       }
+//     }
+//   }
+//
+//
+//   expandValueSet(oid: string, version?: string): Code[] {
+//     const results: Code[] = [];
+//     const valueSet: ValueSet | null = this.findValueSet(oid, version);
+//     if (valueSet == null) {
+//       return [];
+//     }
+//     valueSet.codes.forEach(code => {
+//       if (!results.find(result => {
+//         result === code;
+//       })) {
+//         results.push(code);
+//
+//       }
+//     });
+//     return results;
+//   }
+//
+//
+//   inValueSet(code: Code | Code[], oid: string, version?: string): boolean {
+//     const valueSet: ValueSet | null = this.findValueSet(oid, version);
+//     if (valueSet == null) {
+//       return false;
+//     }
+//
+//     const codeList: Code[] = Array.isArray(code) ? code : [code];
+//
+//     return codeList.every(singleCode => {
+//       valueSet.hasMatch(singleCode);
+//     });
+//   }
+//
+//   anyInValueSet(code: Code | Code[], oid: string, version?: string): boolean {
+//     const valueSet: ValueSet | null = this.findValueSet(oid, version);
+//     if (valueSet == null) {
+//       return false;
+//     }
+//
+//     const codeList: Code[] = Array.isArray(code) ? code : [code];
+//
+//     return codeList.some(singleCode => {
+//       valueSet.hasMatch(singleCode);
+//     });
+//   }
+//
+//
+//   expandCodeSystem(codeSystem: CodeSystem): Code[] {
+//     throw new Error('This function is not implemented.');
+//   }
+//
+//
+//   inCodeSystem(code: Code | Code[], codeSystem: CodeSystem): boolean {
+//     throw new Error('This function is not implemented.');
+//   }
+//
+//   subsumes(subsuming: Code | Concept, subsumed: Code | Concept): boolean {
+//     throw new Error('This function is not implemented.');
+//   }
+// }


Generally speaking, we try to avoid having large blocks of commented out code. If this code is not needed, I'd prefer that we delete it.

cmoesel · 2025-02-14T16:25:50Z

src/cql-code-service.ts

@@ -1,7 +1,7 @@
 import { Code, ValueSet } from './datatypes/datatypes';
 import { TerminologyProvider, ValueSetDictionary, ValueSetObject } from './types';

-export class CodeService implements TerminologyProvider {
+class CodeService implements TerminologyProvider {


In order to remain consistent with the rest of the code base, I think it's best to export the class declaration here (as it was before).

cmoesel · 2025-02-14T16:26:25Z

src/cql-code-service.ts

+//   }
+// }
+
+export { CodeService };


As noted above, in order to remain consistent with the rest of the code base, we should export as part of the class declaration.

cmoesel · 2025-02-14T16:29:06Z

src/elm/expressions.ts

+// TODO Add advanced valueset operations here
+


I think you can delete this.

cmoesel · 2025-02-14T16:29:57Z

src/types/cql-code-service.interfaces.ts

+//
+// /**
+//  * Interface extending TerminologyProvider to include advanced terminology operations such as expanding valuesets and codesystems
+//  */
+//
+// export interface AdvancedTerminologyProvider extends TerminologyProvider {
+//   inValueSet: (code: Code | Code[], oid: string, version?: string) => boolean | Promise<boolean>;
+//   anyInValueSet: (code: Code | Code[], oid: string, version?: string) => boolean | Promise<boolean>;
+//   expandValueSet: ( oid: string, version?: string) => Code[] | Promise<Code[]>;
+//   inCodeSystem: (code: Code | Code[], codeSystem: CodeSystem) => boolean | Promise<boolean>;
+//   // expandCodeSystem: (codeSystem: CodeSystem) => Code[] | Promise<Code[]>;
+//   // subsumes: (subsuming: Code | Concept, subsumed: Code | Concept) => boolean | Promise<boolean>;
+//
+// }


This should be deleted as well.

cmoesel · 2025-02-14T16:44:41Z

I've also just realized that while you added datatype tests, there are no tests for the ELM ExpandValueSet expression. Could you add some cql tests in test/elm/clinical to test expansion of value sets? You could either use the ExpandValueSet CQL operator (if the CQL-to-ELM translator supports it) or you could do a union of value sets to force expansion. There is some information about developing this type of test here, but you can also just look at the existing tests to get a feel for it.

There is one current gotcha. When you run npm run build:test-data, it will generate some invalid ELM changes in the test/elm/type/data.js file due to a bug in CQL-to-ELM. You need to revert those changes for the tests to run (but obviously, keep the changes in test/elm/clinical/data.js.

sorliog · 2025-02-14T18:19:01Z

I've also just realized that while you added datatype tests, there are no tests for the ELM ExpandValueSet expression. Could you add some cql tests in test/elm/clinical to test expansion of value sets? You could either use the ExpandValueSet CQL operator (if the CQL-to-ELM translator supports it) or you could do a union of value sets to force expansion. There is some information about developing this type of test here, but you can also just look at the existing tests to get a feel for it.

There is one current gotcha. When you run npm run build:test-data, it will generate some invalid ELM changes in the test/elm/type/data.js file due to a bug in CQL-to-ELM. You need to revert those changes for the tests to run (but obviously, keep the changes in test/elm/clinical/data.js.

Understood, @cmoesel. I made the requested changes. Let me know if I missed anything.

sorliog added 5 commits February 12, 2025 21:16

Adding support for ExpandValueset by adding AdvancedTerminologyProvid…

d25539d

…er, AdvancedCodeService and ExpandValueSet expression

Removed AdvancedTerminologyProvider and AdvancedCodeService, added ex…

898f8ac

…pand method to valueset for the ExpandValueset operator

Linting code

75cb555

Added Unit tests for Valueset.expand()

f7b377c

Built cql4browsers.js

9688259

sorliog mentioned this pull request Feb 13, 2025

Union operator on valuesets does not work #331

Closed

sorliog added 2 commits February 13, 2025 17:42

Fixed prettier styling

78a8931

Built cql4browsers.js

ef9ff9d

cmoesel requested changes Feb 14, 2025

View reviewed changes

Added more unit tests, fixed styling issues

5ecd084

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added Support for the ExpandValueset operator #332

Added Support for the ExpandValueset operator #332

sorliog commented Feb 13, 2025 •

edited

Loading

codecov-commenter commented Feb 13, 2025 •

edited

Loading

cmoesel left a comment

cmoesel Feb 14, 2025

cmoesel Feb 14, 2025

cmoesel Feb 14, 2025

cmoesel Feb 14, 2025

cmoesel Feb 14, 2025

cmoesel commented Feb 14, 2025

sorliog commented Feb 14, 2025 •

edited

Loading

Added Support for the ExpandValueset operator #332

Are you sure you want to change the base?

Added Support for the ExpandValueset operator #332

Conversation

sorliog commented Feb 13, 2025 • edited Loading

codecov-commenter commented Feb 13, 2025 • edited Loading

Codecov Report

cmoesel left a comment

Choose a reason for hiding this comment

cmoesel Feb 14, 2025

Choose a reason for hiding this comment

cmoesel Feb 14, 2025

Choose a reason for hiding this comment

cmoesel Feb 14, 2025

Choose a reason for hiding this comment

cmoesel Feb 14, 2025

Choose a reason for hiding this comment

cmoesel Feb 14, 2025

Choose a reason for hiding this comment

cmoesel commented Feb 14, 2025

sorliog commented Feb 14, 2025 • edited Loading

sorliog commented Feb 13, 2025 •

edited

Loading

codecov-commenter commented Feb 13, 2025 •

edited

Loading

sorliog commented Feb 14, 2025 •

edited

Loading