hossenlopp commented 4 months ago

Summary

New option to show which clauses are uncovered as a new set of output. Also adds details on coverage including the list of clauses that were not coverage.

Additionally coverage html calculation was made considerably faster (~200x). This should be VERY noticeable when run with larger sets of patients.

New behavior

New options in calculator that are false by default:

calculateClauseUncoverage: Calculates HTML that highlights clauses that are not covered by the set of patients.
calculateCoverageDetails: Calculates the total count of covered and uncovered clauses and lists the location of the uncovered clauses.

These new options will run in the CLI and the coverageDetails will be output as it's own debug output file. This is intended to help with pinpointing coverage issues as it can tell you exactly where in the ELM there are gaps in the coverage.

Code changes

HTMLBuilder.ts - Reworked logic for calculating coverage to collect unique lists of covered and uncovered clause results. Reworked coverage highlighting to work off these smaller lists to vastly speed up HTML generation. Added a helper to highlight specifically for uncoverage. Calculator.ts - Now can handle the two new options and writes out the coverageDetails to a new debug file. types/Calculator.ts - New types for the new options and output. cli.ts - Defaults calculateClauseUncoverage and calculateCoverageDetails to true.

Testing guidance

Test with any measures that feel appropriate and look for any differences in coverage highlighting or percentage. Check to make sure that uncoverage highlighting looks sane.

github-actions[bot] commented 4 months ago

Coverage report

St.^{:grey_question:}	Category	Percentage	Covered / Total
🟢	Statements	85.14% (-0.33% 🔻)	2390/2807
🟡	Branches	73% (-0.28% 🔻)	2223/3045
🟢	Functions	87.47% (-0.68% 🔻)	426/487
🟢	Lines	85.46% (-0.35% 🔻)	2309/2702

Show files with reduced coverage 🔻

|

St.^{:grey_question:}

| File | Statements | Branches | Functions | Lines | | :-: | :- | :- | :- | :- | :- | | 🟢 |

`...` / HTMLBuilder.ts

|

85.71% (-7.25% 🔻)

|

68% (-9.11% 🔻)

|

85.71% (-8.73% 🔻)

|

85.51% (-8.3% 🔻)

|

Test suite run success

451 tests passing in 31 suites.

Report generated by 🧪jest coverage report action from 3c91aa0f105dc9c611daf38cd52d448dde0865c6

lmd59 commented 4 months ago

We're seeing some differences in the "complementary" nature of the coverage highlighting and the uncoverage highlighting. Example w/ all available test patients in ecqm-content-r4-2021/bundles/measure/CervicalCancerScreeningFHIR... Coverage:

Uncoverage:

Might be nice to do a quick pass at re-unhighlighting the first part of the clause, but is not critical to this PR (can be tackled later if needed).

lmd59 commented 3 months ago

We're seeing some differences in the "complementary" nature of the coverage highlighting and the uncoverage highlighting. Example w/ all available test patients in ecqm-content-r4-2021/bundles/measure/CervicalCancerScreeningFHIR... Coverage: Uncoverage:

Might be nice to do a quick pass at re-unhighlighting the first part of the clause, but is not critical to this PR (can be tackled later if needed).

For the same measure, I'm still seeing a small discrepancy in highlighting "complementary-ness" with these two unions. Highlighting may not be the number one priority of this PR, so we might be able to ignore this for now, but do we know what's causing it?

hossenlopp commented 3 months ago

We're seeing some differences in the "complementary" nature of the coverage highlighting and the uncoverage highlighting. Example w/ all available test patients in ecqm-content-r4-2021/bundles/measure/CervicalCancerScreeningFHIR... Coverage: Uncoverage: Might be nice to do a quick pass at re-unhighlighting the first part of the clause, but is not critical to this PR (can be tackled later if needed).

For the same measure, I'm still seeing a small discrepancy in highlighting "complementary-ness" with these two unions. Highlighting may not be the number one priority of this PR, so we might be able to ignore this for now, but do we know what's causing it?

This issue is related to the way chained together unionss create a binary tree and are difficult to highlight as a result. Not all unions have useable localIds or nodes in the ELM annotation sometimes. This is not a new issue created by this PR and measure developers have known to ignore this for years as it is a visual only issue and does not affect coverage calculation. We cannot easily fix this issue and it is possible we need to re-assess this whole matter with new translator output too.

p9g commented 3 months ago

Does the CLI have an option for uncoverage? Can README say something about that?