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.

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

Uncoverage #299

Merged
merged 15 commits into from
Mar 29, 2024
Merged

Uncoverage #299

merged 15 commits into from
Mar 29, 2024

Conversation

hossenlopp
Copy link
Contributor

@hossenlopp hossenlopp commented Mar 14, 2024

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.

Copy link

github-actions bot commented Mar 14, 2024

Coverage report

St.
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.
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 3c91aa0

export interface ClauseCoverageDetails {
totalClauses: number;
coveredClauses: number;
uncoveredClauses: {
Copy link
Contributor

@lmd59 lmd59 Mar 15, 2024

Choose a reason for hiding this comment

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

Because the other two clauses details are numbers, this naming seems a a bit incongruous. It would lead me to expect all of them to be the same type. Maybe we change the other two to totalClausCount and coveredClauseCount or something like that so that it doesn't lead to the expectation that uncoveredClauses also has a count?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good call!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Renamed in ca98110 and added uncovered count

Copy link
Contributor

@elsaperelli elsaperelli left a comment

Choose a reason for hiding this comment

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

Woo uncoverage!!! Super excited for this to be a new part of fqm-e :) Code all looked good to me, I would just suggest adding a section about this to the README.

Another suggestion although I don't know how doable/worth it it would be is to include some sort of "hidden" uncovered clause count? For example, when I run this branch on the MAT6725 measure bundle that was created with the new translator, the uncoverage HTML says 2 uncovered clauses but only one clause is highlighted in red. This is because the other uncovered clause is not a part of the HTML and therefore "hidden". Is there a way we could make this known in our debug output somehow? I think it would be extremely helpful in our future work with coverage highlighting and calculation with the new translator. Also happy to make this an additional task if out of scope.

@elsaperelli elsaperelli self-assigned this Mar 19, 2024
@lmd59
Copy link
Contributor

lmd59 commented Mar 19, 2024

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:
Screenshot 2024-03-19 at 3 06 29 PM
Uncoverage:
Screenshot 2024-03-19 at 3 06 55 PM

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).

@hossenlopp hossenlopp marked this pull request as ready for review March 27, 2024 20:25
Copy link
Contributor

@elsaperelli elsaperelli left a comment

Choose a reason for hiding this comment

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

I still want to do some testing and actually look at the new complementary uncoverage HTML, but I figured I would put up the few comments I have on README wording :)

Yay uncoverage!

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@lmd59
Copy link
Contributor

lmd59 commented Mar 28, 2024

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: Screenshot 2024-03-19 at 3 06 29 PM Uncoverage: Screenshot 2024-03-19 at 3 06 55 PM

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?

Screenshot 2024-03-28 at 1 21 08 PM Screenshot 2024-03-28 at 1 21 01 PM

@hossenlopp
Copy link
Contributor Author

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: Screenshot 2024-03-19 at 3 06 29 PM Uncoverage: Screenshot 2024-03-19 at 3 06 55 PM
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?

Screenshot 2024-03-28 at 1 21 08 PM Screenshot 2024-03-28 at 1 21 01 PM

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.

Copy link
Contributor

@elsaperelli elsaperelli left a comment

Choose a reason for hiding this comment

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

This is an incredible addition to fqm-execution. I did a LOT of testing to look for any discrepancies in the complementary highlighting and to make sure the regular clause coverage highlighting and calculation continued to work the same way. Noting the measures that I tested here for future reference (mostly taken from fqm-execution coverage issues since those measures are the ones that would have benefitted from this feature): CMS104, CMS108, CMS135, CMS144, CMS145, CMS159, CMS165, CMS190, CMS22, CMS334, CMS645, CMS646, CMS69, CMS72, MAT6725.

The only one in which I found an issue with the complementary highlighting was CMS144 and it was the NoBetaBlockerOrdered.medication piece of CQL which we currently have a backlog task to address and therefore I do not think it is an issue in the scope of this PR (just noting for completeness).

Everything makes sense to me! 👏 🥳

@p9g
Copy link

p9g commented Mar 29, 2024

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

@hossenlopp
Copy link
Contributor Author

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

This does run in the CLI when you use --debug. I added info to the README to clarify this in 3c91aa0.

@lmd59 lmd59 merged commit 87aec7b into master Mar 29, 2024
6 checks passed
@lmd59 lmd59 deleted the uncoverage branch March 29, 2024 17:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants