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

Add core__procedure table #329

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Add core__procedure table #329

wants to merge 1 commit into from

Conversation

mikix
Copy link
Contributor

@mikix mikix commented Dec 30, 2024

Puts another brick in the wall of #139

Checklist

  • Consider if documentation in docs/ needs to be updated
    • If you've changed the structure of a table, you may need to run generate-md
    • If you've added/removed core study fields that not in US Core, update our list of those in core-study-details.md
  • Consider if tests should be added
  • Update template repo if there are changes to study configuration in manifest.toml

@@ -45,6 +45,7 @@ in this table.
AND BOOL_OR(ec.table_name = 'documentreference')
AND BOOL_OR(ec.table_name = 'medicationrequest')
AND BOOL_OR(ec.table_name = 'observation')
AND BOOL_OR(ec.table_name = 'procedure')
Copy link
Contributor Author

@mikix mikix Dec 30, 2024

Choose a reason for hiding this comment

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

So I think this deserves a tiny callout.

For context, the way completion works is that we ignore any encounters for which all the listed associated tables have not been loaded by the ETL yet.

So adding a new resource like this to the list of "required resources" means:

  • Transition pain: if a site hasn't ETL'd procedures yet, the next time they rebuild core, all encounters will disappear.
  • Ongoing pain: core requires more and more resources (like procedures) to be exported and ETL'd before it can function as expected

However... This pain is also... the point of completion tracking?

Would we want to draw a line between "resources we really care about" and "resources we kind of care about"? If we didn't add Procedure to the encounter-completion check, but your study used core__procedure, you would now be subject to the reasons we added the completion table - engineers ETL'ing data behind the scenes can cause inconsistent/incomplete results when querying.

This "ignore encounters" trick was so that studies didn't have to know about the whole completion table feature. They just would inner join on encounters at some point and incomplete data would be ignored. But another approach is maybe we have a list of 2nd-tier resources for which studies are expected to manually check the completion tables? (the logic isn't necessarily fun, but we could write some docs with examples)

Or we just accept the pain points listed above as we add resources.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm of two minds about this:

  • Long term, I think this is ok/most correct.
  • Short term, we are getting a lot of partial drips of data, and this makes it harder for folks to spin up/participate

I kinda want to have a discussion at the product level about what's best here and then circle back on the implementation side of things? We can take this as is for now and then backsolve later if we want.

Copy link

github-actions bot commented Dec 30, 2024

☂️ Python Coverage

current status: ✅

Overall Coverage

Lines Covered Coverage Threshold Status
2391 2386 100% 90% 🟢

New Files

File Coverage Status
cumulus_library/studies/core/builder_procedure.py 100% 🟢
TOTAL 100% 🟢

Modified Files

File Coverage Status
cumulus_library/builders/counts.py 100% 🟢
cumulus_library/builders/statistics_templates/counts_templates.py 100% 🟢
cumulus_library/studies/core/count_core.py 100% 🟢
TOTAL 100% 🟢

updated for commit: 542f40d by action🐍

@mikix mikix marked this pull request as ready for review December 30, 2024 15:55
@comorbidity
Copy link
Contributor

Should include Procedure Category , which "should" be implemented at most sites because it is easy, BCH Cerner is not a good indicator for procedure coding because it was DIY home grown.

@comorbidity
Copy link
Contributor

Should include Procedure Category , which "should" be implemented at most sites because it is easy, BCH Cerner is not a good indicator for procedure coding because it was DIY home grown.

STU4 https://hl7.org/fhir/us/core/STU4/StructureDefinition-us-core-procedure.html
https://hl7.org/fhir/R4/valueset-procedure-category.html

@mikix
Copy link
Contributor Author

mikix commented Dec 31, 2024

Should include Procedure Category , which "should" be implemented at most sites because it is easy, BCH Cerner is not a good indicator for procedure coding because it was DIY home grown.

Done!

@@ -45,6 +45,7 @@ in this table.
AND BOOL_OR(ec.table_name = 'documentreference')
AND BOOL_OR(ec.table_name = 'medicationrequest')
AND BOOL_OR(ec.table_name = 'observation')
AND BOOL_OR(ec.table_name = 'procedure')
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm of two minds about this:

  • Long term, I think this is ok/most correct.
  • Short term, we are getting a lot of partial drips of data, and this makes it harder for folks to spin up/participate

I kinda want to have a discussion at the product level about what's best here and then circle back on the implementation side of things? We can take this as is for now and then backsolve later if we want.

--
-- AND ADDING:
-- * the `category` field, because it's helpful for classification
-- * the `encounter` field, because come on, why is it left out of the US Core profile
Copy link
Contributor

Choose a reason for hiding this comment

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

you've got to be kidding me

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.

3 participants