Skip to content

multicontext values for cli tool #652

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

Closed
Capt-Mac wants to merge 1 commit into from
Closed

multicontext values for cli tool #652

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
21 changes: 13 additions & 8 deletions cqf-fhir-cr-cli/src/main/java/org/opencds/cqf/fhir/cr/cli/command/CqlCommand.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,15 +96,15 @@ static class LibraryParameter {
@Option(names = {"-e", "--expression"})
public String[] expression;

@ArgGroup(multiplicity = "0..1", exclusive = false)
@ArgGroup(exclusive = false)
public ContextParameter context;

@brynrhodes brynrhodes Apr 8, 2025 •
edited
Loading

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.

Isn't the multiplicity change going to affect the cardinality of the context field? So this should be public List<ContextParameter> contexts?

@Capt-Mac Capt-Mac Apr 8, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It currently reuses the singular context for each provided context-value. Wouldn't the cql file only have a single context?

@brynrhodes brynrhodes Apr 9, 2025

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.

If that's how the command option parameters work, then okay, it just seems like if the multiplicity is on the "context" element, then that's the thing that would end up needing to be able to be list-valued? Maybe I just don't understand how the command option parameters are working.

@Capt-Mac Capt-Mac Apr 9, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Screenshot 2025-04-09 at 9 04 19 AM

This should allow for behavior like this:
--context Patient --context-value id1 --context-value id2 --context-value id3

I confirmed this behavior with the unit test. You cannot add multiple contextName values

@Capt-Mac Capt-Mac Apr 9, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

we could add a second test where two contextNames are added which should cause a failure

@JPercival JPercival Apr 9, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Actually, Bryn's right here. A given CQL library could have more than one context (some CQL in Encounter context, some in Patient) each of which would need to be set. We don't have a way to correlate across multiple contexts, so the name=value pairs are with respect to each Library evaluation.

@JPercival JPercival Apr 9, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

IOW, the CLI is currently set up to so that you parameterize one evaluation:

  • the library
  • the expressions
  • the parameters
  • the context parameters

It's not set up for multiple evaluations of a single library with different parameters.

I think this is a desirable behavior for this command. I think we need a new, different command with different semantics.

@JPercival JPercival Apr 9, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Either that or reorganize the arguments so that you specify the library/expression independently of the rest of the parameters.


static class ContextParameter {
@Option(names = {"-c", "--context"})
public String contextName;

@Option(names = {"-cv", "--context-value"})
public String contextValue;
public List<String> contextValues;
}

static class ModelParameter {
Expand Down Expand Up @@ -196,8 +196,7 @@ public Integer call() throws Exception {
evaluationSettings.setNpmProcessor(new NpmProcessor(igContext));

for (LibraryParameter library : libraries) {
var repository = createRepository(
fhirContext, library.terminologyUrl, library.model.modelUrl, library.context.contextValue);
var repository = createRepository(fhirContext, library.terminologyUrl, library.model.modelUrl, null);
var engine = Engines.forRepository(repository, evaluationSettings);

if (library.libraryUrl != null) {
Expand All @@ -213,12 +212,18 @@ public Integer call() throws Exception {
Pair<String, Object> contextParameter = null;

if (library.context != null) {
contextParameter = Pair.of(library.context.contextName, library.context.contextValue);
}
for (String contextValue : library.context.contextValues) {
contextParameter = Pair.of(library.context.contextName, contextValue);
EvaluationResult result = engine.evaluate(identifier, contextParameter);
writeResult(result);
}

EvaluationResult result = engine.evaluate(identifier, contextParameter);
} else {

writeResult(result);
EvaluationResult result = engine.evaluate(identifier, contextParameter);

writeResult(result);
}
}

return 0;
Expand Down
31 changes: 27 additions & 4 deletions cqf-fhir-cr-cli/src/test/java/org/opencds/cqf/fhir/cr/cli/CliTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -410,8 +410,8 @@ void qICoreSupplementalDataElements() {
output.contains(
"SDE Race=[Code { code: 1586-7, system: urn:oid:2.16.840.1.113883.6.238, version: null, display: Shoshone }, Code { code: 2036-2, system: urn:oid:2.16.840.1.113883.6.238, version: null, display: Filipino }, Code { code: 1735-0, system: urn:oid:2.16.840.1.113883.6.238, version: null, display: Alaska Native }]"));
assertTrue(output.contains("SDE Payer=[Tuple {\n" + " code: Concept {\n"
+ "\tCode { code: 59, system: urn:oid:2.16.840.1.113883.3.221.5, version: null, display: Other Private Insurance }\n"
+ "}\n" + " period: Interval[2011-05-23, 2012-05-23]\n" + "}]"));
+ "\tCode { code: 59, system: urn:oid:2.16.840.1.113883.3.221.5, version: null, display: Other Private Insurance }\n"
+ "}\n" + " period: Interval[2011-05-23, 2012-05-23]\n" + "}]"));
assertTrue(
output.contains(
"SDE Sex=Code { code: M, system: http://hl7.org/fhir/v3/AdministrativeGender, version: null, display: Male }"));
Expand Down Expand Up @@ -463,7 +463,7 @@ void qICoreEXM124Denom() {
"-ln=EXM124_QICore4",
"-lv=8.2.000",
"-m=FHIR",
"-mu=" + testResourcePath + "/qicore/denom-EXM124",
"-mu=" + testResourcePath + "/qicore/tests/denom-EXM124",
"-t=" + testResourcePath + "/qicore/vocabulary/valueset",
"-c=Patient",
"-cv=denom-EXM124"
Expand Down Expand Up @@ -491,7 +491,7 @@ void qICoreEXM124Numer() {
"-ln=EXM124_QICore4",
"-lv=8.2.000",
"-m=FHIR",
"-mu=" + testResourcePath + "/qicore/numer-EXM124",
"-mu=" + testResourcePath + "/qicore/tests/numer-EXM124",
"-t=" + testResourcePath + "/qicore/vocabulary/valueset",
"-c=Patient",
"-cv=numer-EXM124"
Expand All @@ -510,6 +510,29 @@ void qICoreEXM124Numer() {
assertTrue(output.contains("Numerator=true"));
}

@Test
void qICoreMultiContext() {
String[] args = new String[] {
"cql",
"-fv=R4",
"-lu=" + testResourcePath + "/qicore",
"-ln=EXM124_QICore4",
"-lv=8.2.000",
"-m=FHIR",
"-mu=" + testResourcePath + "/qicore/tests",
"-t=" + testResourcePath + "/qicore/vocabulary/valueset",
"-c=Patient",
"-cv=numer-EXM124",
"-cv=denom-EXM124"
};

Main.run(args);

String output = outContent.toString();
assertTrue(output.contains("Patient=Patient(id=numer-EXM124)"));
assertTrue(output.contains("Patient=Patient(id=denom-EXM124)"));
}

@Test
@Disabled("This test is failing on the CI Server for reasons unknown. Need to debug that.")
void sampleContentIG() {
Expand Down
0 ...enom-EXM124/encounter/denom-EXM124-1.json → ...enom-EXM124/encounter/denom-EXM124-1.json
View file
Open in desktop
File renamed without changes.
0 ...om-EXM124/observation/denom-EXM124-2.json → ...om-EXM124/observation/denom-EXM124-2.json
View file
Open in desktop
File renamed without changes.
0 ...re/denom-EXM124/patient/denom-EXM124.json → ...ts/denom-EXM124/patient/denom-EXM124.json
View file
Open in desktop
File renamed without changes.
0 ...umer-EXM124/encounter/numer-EXM124-1.json → ...umer-EXM124/encounter/numer-EXM124-1.json
View file
Open in desktop
File renamed without changes.
0 ...er-EXM124/observation/numer-EXM124-2.json → ...er-EXM124/observation/numer-EXM124-2.json
View file
Open in desktop
File renamed without changes.
0 ...re/numer-EXM124/patient/numer-EXM124.json → ...ts/numer-EXM124/patient/numer-EXM124.json
View file
Open in desktop
File renamed without changes.