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

c_us_core_v4_count: expand mandatory fields to their own table(s) #44

Merged
merged 3 commits into from
Jun 4, 2024

Conversation

mikix
Copy link
Contributor

@mikix mikix commented May 31, 2024

We currently combine all the mandatory checks into one boolean field: valid_mandatory.

The qualifier metric definition encourages stratifying by all mandatory and must support fields.

This commit makes the following changes:

  • Adds a _must_support suffix to the various c_us_core_v4_{profile} tables
    • Drops valid_mandatory (now expanded & split into new table)
  • Adds a new c_us_core_v4_{profile}_mandatory table that cubes all the individual mandatory checks (as well as an AND'd valid field)
  • For the observation based profiles, additionally chop up the mandatory table into two or three tables in cube mode - and only in cube mode - for performance reasons
    • i.e. we see tables like data_metrics__c_us_core_v4_count_observation_laboratory_mandatory1 and data_metrics__c_us_core_v4_count_observation_laboratory_mandatory2
    • Laboratory and smoking status are split into two tables, with three for vital signs
    • This seemed like a better trade off than dropping any fields
    • And it's isolated to cube mode. In aggregate mode, we still use one mandatory table.

Fixes #40.

All mandatory checks used to be summarized in a single
'valid_mandatory' field. But it's useful to see where the faults lie.

So now each resource has a _mandatory and a _must_support table (for
CUBE performance reasons) and each table has a column for every
relevant field.
@mikix
Copy link
Contributor Author

mikix commented May 31, 2024

This is all maybe an argument that even for Cumulus dashboard, we should be pushing up the aggregate mode output for this metric. But that will have to wait until the dashboard can actually show aggregate mode tables.

In cube mode, split the large Observation mandatory tables up into two
or three tables. In aggregate mode, we leave them all as one table.

This is just a clunky performance workaround, but we can at least
isolate it to cube mode.
Comment on lines +79 to +80
# TODO: add the ability for cumulus-library to take study args like
# --study-option=output-mode:cube (or whatever)

Choose a reason for hiding this comment

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

i know this is on our short list but for posterity: smart-on-fhir/cumulus-library#247

@gotdan
Copy link

gotdan commented Jun 4, 2024

@mikix, it looks like verification_status is showing up as both mandatory and must support for allergyintolerance and condition, but seems like it should just be in must support?

@mikix
Copy link
Contributor Author

mikix commented Jun 4, 2024

@mikix, it looks like verification_status is showing up as both mandatory and must support for allergyintolerance and condition, but seems like it should just be in must support?

Ah that's an odd thing I guess. Here's a note I have in the must-support SQL:

        -- This is a repeated check with the mandatory side.
        -- We check the code binding on the mandatory side, since that's a required binding
        -- and we like to validate those where possible.
        -- But this field is really only a must-support field, so we also report it here.

So it's a required binding on a must-support field. If the value of that field is bogus, do we flag that as breaking a mandatory constraint or just report it on the must-support side of things?

(The mandatory check allows NULL - the must-support check does not - so they are slightly different. Mandatory doesn't care if the field is present, only that it's well-formed. But must-support is checking that it's present & well-formed.)

And keep in mind the q_valid_us_core_v4 side of things too - if the value of that field is bogus, should it pass mandatory validation or not?

@gotdan
Copy link

gotdan commented Jun 4, 2024

Yup, I certainly agree that validating the code should be considered mandatory from a q_valid_us_core_v4 perspective. On the c_us_core_v4 side of things, I think it would be less confusing to just report it in must-support (populated and valid), but I don't feel strongly if your preferences is to keep it in both places.

@mikix
Copy link
Contributor Author

mikix commented Jun 4, 2024

Maybe just a field rename would resolve this easily - call it valid_verification_status_binding on the mandatory side maybe.

@mikix
Copy link
Contributor Author

mikix commented Jun 4, 2024

Eh, it's easy enough to skip it in the c_us_core_v4_count case - I can do that real quick

@mikix mikix merged commit 9b1bd1e into main Jun 4, 2024
2 checks passed
@mikix mikix deleted the mikix/mandatory-fields branch June 4, 2024 15:13
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.

c_us_core_v4_count: expand all mandatory fields
3 participants