Support for multiple initial populations

[mgramigna]

Summary

Fixes #149

This PR enables the results processing logic of fqm-execution to handle the case of multiple Initial Populations defined in a measure. Note that this is only applicable for Ratio Measures.

For context, in the case of a ratio measure when there are two IPs, the numerator and the denominator are treated independently when they depend on different IPs.

New behavior

• Ratio measures with 2 IPs no longer have numerator/denominator dependencies
  • Results for each IP are reported in populationResults and episodeResults where appropriate
• Population relevance has special case handing for multiple IP measures
• Results and relevance in these cases are determined by looking at the criteriaReference extension in the numer/denom population
• Episode-based measures can now be detected by the cqfm-populationBasis extension being present at the individual population level (as opposed to Measure or Measure.group)

Code changes

• Update clause results builder for relevants/results post-processing to only have the numerator and denominator influence each other when we are not in a multiple-ipp case
• Update all the results processing to use criteriaExpression when provided to resolve ambiguity between multiple populations of the same type
• Update episode of care group checking to have special handling for ratio measures, allowing populationBasis to be specified at the population level
• Unify all uses of checking for scoringCode to helper functions
• Add tests for everything
• Unify the two different MeasureBundleHelper test files into 1

Testing guidance

1. Grab the bundle and patient from Ratio second IP missing episode based results and population referencing second IP has incorrect result #149 and run the CLI, ensuring that the results are as expected:

npm run cli -- detailed -m measure-bundle.json -p patient-bundle.json

Verify the differences with the new output and what was there on master before. You should see that ipp2 is now properly being reported as true

2. Download multiple-ipp-tests.zip and extract the individual files within it to the root of fqm-execution. These are test cases I created and bundled that further test this logic, specifically testing the "independentness" of numer and denom in these multiple IPP cases. Instructions are in MULTIPLE-IP-INSTRUCTIONS.md, but it's a two parter:

Part 2A: Test cases

Run the ./run-multiple-ip-test-cases.sh file to generate 3 JSON outputs for each of the CQL libraries present in the multiple-ipp-test-cases directory. Please cross-reference the results with the CQL files.

• false-denom-output.json - Of note in this is that ipp and denom both have no results, but ipp2 and numer have one episode result. you should see that the results for ipp2 and numer are reported as true independent of ipp and denom. This would not happen on master. In addition, denom is not relevant, as its corresponding ipp is falsy. numerator is relevant though, since its corresponding ipp is truthy
• false-numer-output.json - Of note in this one is that the numerator population is not relevant since its ipp is false, but denom/ipp are both relevant and truthy
• both-true-output.json - Everything is true and relevant, and we get results for 2 episodes

Part 2B: Regression Tests

Given that a lot of the results processing has changed in this PR, I wrote some scripts that ensure that previous calculation behavior for measures without two IPPs is unchanged.

• Clone connectathon to the root of fqm-execution if you don't have it: git clone https://github.com/DBCG/connectathon.git
• Run the regression script: ./run-regression.sh

PASS means that there are 0 diffs in the detailed results for that measure/test case between master and this branch. This is expected, as none of the measures in connectathon have two IPPs, so calculation results should be unchanged.

[github-actions]

Coverage report

St.❔	Category	Percentage	Covered / Total
🟢	Statements	83.15% (+0.45% 🔼)	1836/2208
🟡	Branches	70.86% (+0.83% 🔼)	1588/2241
🟢	Functions	85.04% (+0.63% 🔼)	324/381
🟢	Lines	83.4% (+0.42% 🔼)	1769/2121

Show files with reduced coverage 🔻

St.❔	File	Statements	Branches	Functions	Lines
🟡	... / MeasureReportBuilder.ts	71.69% (-0.21% 🔻)	54.75% (-1.88% 🔻)	67.19% (-0.99% 🔻)	74.59% (-0.21% 🔻)

Test suite run success

320 tests passing in 28 suites.

Report generated by 🧪jest coverage report action from a206b7f

[elsaperelli]

Nice! Just some small changes and questions. My only overall suggestion is to maybe add some unit tests for DetailedResultsBuilder.ts since there were some changes there that are not covered? Unsure how necessary that is though with all the external tests you made.

[mgramigna]

Nice! Just some small changes and questions. My only overall suggestion is to maybe add some unit tests for DetailedResultsBuilder.ts since there were some changes there that are not covered? Unsure how necessary that is though with all the external tests you made.

I Added some more tests for the branch of logic that I changed in handlePopuationValues of DetailedResultsBuilder. I didn't go full crazy and add tests for everything in this function since it was completely untested (I was tempted, but I think it's a little out of scope for this)

Ideally we can expand the tests more in the future.

This led to a net increase in test coverage for that file, so I think that's good

[elsaperelli]

Lgtm! Thank you for adding the detailed comments! Those will be helpful. As well as the additional tests.

[hossenlopp]

From a quick pass, the approach is sound. 👍

I didn't see anything relevant to making sure the distinct IPPs are getting results in the MeasureReportBuilder. Maybe it already is inherently doing it right.

[mgramigna]

From a quick pass, the approach is sound. 👍

I didn't see anything relevant to making sure the distinct IPPs are getting results in the MeasureReportBuilder. Maybe it already is inherently doing it right.

Yeah, I think we will have the same problem as we do with ratio measures with multiple observations: need to differentiate them in the MeasureReport. We're avoiding this for now. If this ticket gets in, we can probably use this to differentiate the populations. For now, I wonder if (as a separate PR) we should update the MeasureReportBuilder to either throw an error or a warning or something when we encounter one of these edge cases, just so users don't expect it to work (and we can recommend using the low-level detailed results API or something)

[natjoe4]

Tested thoroughly and works well! Took a good look at the code diff and only have a couple tiny comments. Nice work!

Requested changes are non-issues