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

"type": "Equivalent" Should not contain a localId #1279

Closed
RohitKandimalla opened this issue Nov 13, 2023 · 8 comments
Closed

"type": "Equivalent" Should not contain a localId #1279

RohitKandimalla opened this issue Nov 13, 2023 · 8 comments
Assignees
Labels
bug engine impact The issue has the potential to have an impact on ELM engine evaluation md impact tracking The issue is impacting measure development

Comments

@RohitKandimalla
Copy link

RohitKandimalla commented Nov 13, 2023

Hello Team,

BonnieMAT team received a help desk ticket where user is unable to achieve 100% test coverage. After investigating we found out that the ELM generated for that particular measure is including a localId property for "type": "Equivalent" when followed by a "NOT" (negation). Ideally localId should not be added as it is causing issues downstream.

Original HelpDesk ticket : BONNIEMAT-1658

Original Measure bundle exported from MAT, it contains the CQL for the measure library as well as all included libraries.
CMS137-v12-0-001-QDM-5-6.zip

CQL definition where it is used

define "Treatment Initiation With Non Medication Intervention Dates":
  "Psychosocial Visit" PsychosocialVisit
    let treatmentDate: date from start of Global."NormalizeInterval" ( PsychosocialVisit.relevantDatetime, PsychosocialVisit.relevantPeriod )
    with "First SUD Episode During Measurement Period" FirstSUDEpisode
      such that treatmentDate during Interval[date from start of FirstSUDEpisode.relevantPeriod, date from start of FirstSUDEpisode.relevantPeriod + 14 days )
        and PsychosocialVisit.id !~ FirstSUDEpisode.id
    return all treatmentDate

Corresponding ELM

{
      "localId": "212",
      "locator": "133:13-133:54",
      "type": "Not",
      "operand":
      {
          "localId": "211",
          "locator": "133:13-133:54",
          "type": "Equivalent",
          "operand":
          [
              {
                  "localId": "208",
                  "locator": "133:13-133:32",
                  "path": "id",
                  "scope": "PsychosocialVisit",
                  "type": "Property"
              },
              {
                  "localId": "210",
                  "locator": "133:37-133:54",
                  "path": "id",
                  "scope": "FirstSUDEpisode",
                  "type": "Property"
              }
          ]
      }
  }

As we can see in the ELM, Equivalent operator is followed by a negation ( NOT ), in this case having a localID ("localId": "211") for Equivalent object is causing issues downstream.

Note: When CQL is modified to check equal condition instead of equivalent PsychosocialVisit.id != FirstSUDEpisode.id then the ELM generated doesn't contain a localID and hence the code coverage is not effected.

Please let us know if you need any additional information, Happy to provide !!

@JPercival JPercival added bug input needed The issue needs more input from the submitter or another participant labels Nov 15, 2023
@JPercival
Copy link
Contributor

It looks to me like the localId is correctly emitted there. Not and Equivalent are both first-class operators in the CQL spec. Consider the following examples:

Emits localId for equal:

define "Test":
    [Observation] O where O.status.value = 'final'

Emits localId for equivalent:

define "Test":
    [Observation] O where O.status.value ~ 'final'    

Does NOT emit localId for equal:

define "Test":
    [Observation] O where O.status.value != 'final'    

Does emit localId for equivalent:

define "Test":
    [Observation] O where O.status.value !~ 'final'    

So, I actually think the bug is the opposite of what you've posted. For some reason the localId is not being emitted when negating Equals when in fact it should be.

@brynrhodes what are your thoughts?

@JSRankins
Copy link
Contributor

@JPercival , I was wondering that as well when we were looking at this. It appears that cqm-execution and fqm-execution are ironically providing the anticipated behavior for coverage calculation when the localId is missing for "Equivalent" first-class operator. But I think that's because the localId for the "Equivalent" expression is not in the annotations.

Annotation from CMS137-v12-0-001-QMD-5-6.json (from CMS137-v12-0-001-QDM-5-6.zip starting around line 3083):
{ "r" : "212", "s" : [ { "r" : "208", "s" : [ { "r" : "207", "s" : [ { "value" : [ "PsychosocialVisit" ] } ] }, { "value" : [ "." ] }, { "r" : "208", "s" : [ { "value" : [ "id" ] } ] } ] }

But the localId is present within the related expression block as @RohitKandimalla noted (starting around line 3262).
{ "localId" : "212", "locator" : "133:13-133:54", "type" : "Not", "operand" : { "localId" : "211", "locator" : "133:13-133:54", "type" : "Equivalent", "operand" : [ {

As a result, there is nothing to tie 211 back to, and coverage is impacted.

@brynrhodes
Copy link
Member

I agree that localIds should be being emitted, and I think you're right @JPercival , that the fact that it's not being emitted for Equivalent is actually a bug. However, I think what Stan is pointing out is that the annotations aren't consistent, in that they are referencing a localId that doesn't exist. So I think there are two issues here to be addressed.

@brynrhodes brynrhodes added md impact tracking The issue is impacting measure development engine impact The issue has the potential to have an impact on ELM engine evaluation and removed input needed The issue needs more input from the submitter or another participant labels Nov 22, 2023
@brynrhodes
Copy link
Member

I tagged with md impact tracking (because this is affecting coverage calculation) and with engine impact (because it impacts how annotations and localIds are being output which affects coverage calculation).

@JPercival
Copy link
Contributor

Related to: Related to #1279, #1131, #704

Expected root cause fix is: #697

@EvanMachusak
Copy link

I would think implementers of cql-to-elm are free to assign local IDs to all elements. It is a property of the Element type in the schema.

@JPercival
Copy link
Contributor

Also related #1295

@birick1
Copy link

birick1 commented Dec 6, 2023

For fqm-execution, we also encountered the coverage calculation behavior noted in the original post in projecttacoma/fqm-execution#287.

In that ticket, the third measure - CMS69 - uses a !~ which affects the coverage calculation. In short, because the ELM for !~ is split into both ELM clauses Not and Equivalent, getting coverage requires two test cases:

  • the objects aren't equivalent (to satisfy ELM Not)
  • the objects are equivalent (to satisfy ELM Equivalent)

The need for both test cases is the reason why 100% isn't achieved:

BonnieMAT team received a help desk ticket where user is unable to achieve 100% test coverage. After investigating we found out that the ELM generated for that particular measure is including a localId property for "type": "Equivalent" when followed by a "NOT" (negation). Ideally localId should not be added as it is causing issues downstream.

Having the localId for Equals omitted in the ELM for != looks like it works, but is really just masking the above issue for the coverage calculation when using the != operator. Once the localId is emitted, != and !~ will have the same behavior in the coverage calculation and need two test cases.

To address needing two test cases when using !~ or != (assuming a localId will be emitted in the future), we've drafted PR projecttacoma/fqm-execution#291 to detect the use of !~ and !=. When those cases are detected, only a test case checking the objects are not equal/not equivalent will be needed. We are still testing/reviewing this PR, and would welcome feedback.

Note: fqm-execution is only for FHIR measures. The original issue in this ticket looks like it is for a QDM measure. I haven't verified that QDM measures needed both test cases like the FHIR measures do, but the behavior sounds the same.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug engine impact The issue has the potential to have an impact on ELM engine evaluation md impact tracking The issue is impacting measure development
Projects
None yet
Development

No branches or pull requests

6 participants