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

Updated discovery for new FHIR resources #300

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

dogversioning
Copy link
Contributor

This adds allergyintolerance, diagnosticreport, and procedure to resources the discovery study checks for coding information.

It also lightly redoes test data generation/validation logic due to some issues with commas inside fields.

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

Copy link

github-actions bot commented Sep 26, 2024

☂️ Python Coverage

current status: ✅

Overall Coverage

Lines Covered Coverage Threshold Status
2240 2228 99% 90% 🟢

New Files

No new covered files...

Modified Files

File Coverage Status
cumulus_library/studies/discovery/code_definitions.py 100% 🟢
TOTAL 100% 🟢

updated for commit: aa065fc by action🐍

@@ -178,6 +178,7 @@ def debug_table_head(cursor, table, rows=3, cols="*"):
def debug_diff_tables(cols, found, ref, pos=0):
cols = cols if len(cols) > 0 else []
found = found[pos] if len(found) > pos else []
print(str(ref))
Copy link
Contributor

Choose a reason for hiding this comment

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

✂️ ?

Comment on lines 49 to 50
for i in range(0, len(table_rows)):
table_rows[i] = tuple([x if x else "" for x in table_rows[i]])
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: it's odd to my Python eyes to see an actual index loop 😄 maybe something like this? Could be cut over several lines for clarity.

table_rows = [tuple(x or "" for x in row) for row in table_rows]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i personally find this more obtuse, but i agree it's more pythonic, so i'll make this change.

Copy link
Contributor

Choose a reason for hiding this comment

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

I only generally like that construction with good formatting:

table_rows = [
    tuple(x or "" for x in row)
    for row in table_rows
]

Which I find readable enough - but if you vibe your approach for readability, I think that's a good enough reason to keep it as-is.

Comment on lines 55 to 60
"/test_data/discovery/discovery__code_sources.csv",
"w",
) as f:
writer = csv.writer(f, quoting=csv.QUOTE_MINIMAL)
for row in table_rows:
writer.writerow(row)
Copy link
Contributor

Choose a reason for hiding this comment

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

Should comment this block out yeah?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I could've sworn i'd done this.

Comment on lines 15 to 17
'',
'',
''
Copy link
Contributor

Choose a reason for hiding this comment

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

This reference file has a lot of blanks - is there no coding data in the test data for allergies or these others? Procedure.reasonCode has something. But it's a rarity. Even encounter.hospitalization.dischargedisposition lost data.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is probably due to running against my test db - my internet access has been so bad from home over the last few days on VPN that i got bizaare timeouts trying to run this against cerner. I'll try it again tomorrow when i'm onsite.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think in CONTRIBUTING.md, we have a one-liner to run against the test data - does that generate good-enough results?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated w/ local db - still pretty blank-ful.

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.

2 participants