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

Look for human-readable names for conditions #3112

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

Conversation

JNygaard-Skylight
Copy link
Collaborator

PULL REQUEST

Summary

The message-parser config the seed script uses is not getting human readable names.

Related Issue

Fixes # n/a

Acceptance Criteria

Please copy the acceptance criteria from your ticket and paste it here for your reviewer(s)

Additional Information

Anything else the review team should know?

Checklist

  • If this code affects the other scrum team, have they been notified? (In Slack, as reviewers, etc.)

Copy link

codecov bot commented Jan 8, 2025

Codecov Report

Attention: Patch coverage is 82.60870% with 8 lines in your changes missing coverage. Please review.

Project coverage is 91.53%. Comparing base (f9f8818) to head (c260c3a).
Report is 832 commits behind head on main.

Files with missing lines Patch % Lines
containers/trigger-code-reference/app/utils.py 63.63% 8 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3112      +/-   ##
==========================================
+ Coverage   87.03%   91.53%   +4.50%     
==========================================
  Files         221      118     -103     
  Lines       13664     7074    -6590     
  Branches      708      738      +30     
==========================================
- Hits        11892     6475    -5417     
+ Misses       1763      590    -1173     
  Partials        9        9              
Flag Coverage Δ
ecr-viewer 91.10% <ø> (+0.11%) ⬆️
fhir-converter ?
ingestion ?
message-parser 96.53% <ø> (ø)
message-refiner ?
orchestration 85.67% <ø> (ø)
record-linkage ?
trigger-code-reference 86.71% <82.60%> (-0.15%) ⬇️
validation ?

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
containers/trigger-code-reference/app/main.py 90.66% <100.00%> (+0.25%) ⬆️
...tainers/trigger-code-reference/tests/test_utils.py 100.00% <100.00%> (ø)
containers/trigger-code-reference/app/utils.py 90.47% <63.63%> (-2.31%) ⬇️

... and 104 files with indirect coverage changes

@JNygaard-Skylight JNygaard-Skylight marked this pull request as draft January 10, 2025 17:45
…jnygaard-skylight:CDCgov/phdi into josh/message-parser-readable-conditions
@JNygaard-Skylight JNygaard-Skylight marked this pull request as ready for review January 16, 2025 16:17
@@ -349,7 +349,7 @@
}
},
"condition": {
"fhir_path": "Observation.valueCodeableConcept.coding.display",
"fhir_path": "iif(Observation.valueCodeableConcept.text.exists(), Observation.valueCodeableConcept.text, Observation.valueCodeableConcept.coding.display)",
Copy link
Collaborator

Choose a reason for hiding this comment

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

does the display need a first since there can be multiple?

None,
)

if condition_code:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
if condition_code:
if condition_code is not None:

Comment on lines +266 to +267
elif "display" in condition_code:
resource["valueCodeableConcept"]["text"] = condition_code["display"]
Copy link
Collaborator

Choose a reason for hiding this comment

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

what's the thinking around including the display as a fallback? the display would be in the resulting fhir bundle either way, right? so it's not stamping new/outside info


Example:
$ python seed_rckms_database.py <archive.zip> <database.db>
A significantly paired down version of the original script to seed the RCKMS database. This script will extract the conditions from the RCKMS CSV in assets (downloaded from https://www.rckms.org/content-repository) files and load them into the SQLite database.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
A significantly paired down version of the original script to seed the RCKMS database. This script will extract the conditions from the RCKMS CSV in assets (downloaded from https://www.rckms.org/content-repository) files and load them into the SQLite database.
A significantly pared down version of the original script to seed the RCKMS database. This script will extract the conditions from the RCKMS CSV in assets (downloaded from https://www.rckms.org/content-repository) files and load them into the SQLite database.

"VALUES (:snomed, 'http://snomed.info/sct', :name) "
"ON CONFLICT DO NOTHING",
condition.__dict__,
"INSERT INTO conditions (id, system, name, description) VALUES (?, ?, ?, ?)",
Copy link
Collaborator

Choose a reason for hiding this comment

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

do we ever get conflicts in practice?

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