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

FHIR feature updates #285

Merged
merged 4 commits into from
May 14, 2024
Merged

FHIR feature updates #285

merged 4 commits into from
May 14, 2024

Conversation

joeflack4
Copy link
Collaborator

@joeflack4 joeflack4 commented Jul 11, 2022

Partially addresses:

Overview

  • Minor bug fixes
  • Dev updates (comments, etc)
  • Added test(s)

Updates

7b91d57da9a5143c5ce8335897ad2542424797c6

    - Add: _test_to_fhir_json()

157ae226921891feeaa38b4329c98b762b2b8727

    - Refactored 3 JSON writer functions to a state that they were probably intended to be in: 1 json function with branching logic depending on 'serialization' param.

0ab8625887a1d57adeadd5ad6d3c47dd08524839

    - Update: Minor codestyle updates, comments, todo's
    - Delete: Unused schema/fhir_json.csv. Mappings will continue to be done via pure Python
    - Bugfix: extension.ValueString -> extension.valueString
    - Delete: Comments about spec field mappings

0dd0772dc7bc4a263fa8e8f8e5c02c6f6c3264bf

    - Added: schema/fhir_json.csv: For automation of conversion by utilizing CSV export from curated GoogleSheet.
    - Update: Comments: Added state of mappings between SSSOM and FHIR ConceptMap

@joeflack4 joeflack4 self-assigned this Jul 11, 2022
@joeflack4 joeflack4 marked this pull request as draft July 11, 2022 21:46
@joeflack4 joeflack4 requested a review from matentzn July 11, 2022 21:47
@joeflack4 joeflack4 added the enhancement New feature or request label Jul 11, 2022
@matentzn

This comment was marked as outdated.

sssom/writers.py Outdated Show resolved Hide resolved
@joeflack4 joeflack4 changed the title FHIR Feature FHIR feature updates Aug 10, 2022
schema/fhir_json.csv Outdated Show resolved Hide resolved
sssom/writers.py Outdated Show resolved Hide resolved
@joeflack4 joeflack4 force-pushed the fhir-extension branch 11 times, most recently from 40141eb to f6fed2e Compare September 4, 2023 00:49
@@ -450,7 +520,7 @@ def to_fhir_json(msdf: MappingSetDataFrame) -> Dict:
# ...that is, if I happen to know the categories/codes for this categorical variable
# ...if i do that, do i also need to upload that coding as a (i) `ValueSet` resource? (or (ii) codeable concept? prolly (i))
"url": "http://example.org/fhir/StructureDefinition/mapping_justification",
"ValueString": row.get(
"valueString": row.get(
Copy link
Collaborator Author

@joeflack4 joeflack4 Sep 4, 2023

Choose a reason for hiding this comment

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

Capitalization bugfix

Partially addresses:

@matentzn This is an FYI, although this is the main reason I think I started this PR. It's the only bug fix.

@ShahimEssaid This is a main reason of this PR. But I wanna get this one merged first to get a flow going; more updates will come in subsequent PRs.

src/sssom/writers.py Outdated Show resolved Hide resolved
src/sssom/writers.py Outdated Show resolved Hide resolved
@matentzn matentzn requested review from hrshdhgd and removed request for matentzn September 19, 2023 13:53
tests/test_cli.py Show resolved Hide resolved
src/sssom/writers.py Show resolved Hide resolved
@joeflack4 joeflack4 force-pushed the fhir-extension branch 3 times, most recently from bc3c37e to 6e1b54f Compare May 5, 2024 04:52
self.assertEqual(d["url"], mapping_set_id)
self.assertIn(d["identifier"][0]["system"], mapping_set_id)
self.assertEqual(d["identifier"][0]["value"], mapping_set_id)
self.assertEqual(d["identifier"][0]["value"], d["url"])
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Update: test_write_sssom_fhir()

Added more assertions. @ShahimEssaid could you maybe review my assertions and, for whichever you can easily understand, can you confirm if my expectations of what I should be seeing in the output are valid / ideal? Feel free to suggest any other assertions I might want to add.

Note that I don't have any outputs for you to review yet. When I do, they will be in: https://github.com/timsbiomed/sssom-on-fhir-content/

FYI here's the actual output file that I'm testing here: basic.fhir.json

tests/test_writers.py Outdated Show resolved Hide resolved
@joeflack4 joeflack4 force-pushed the fhir-extension branch 2 times, most recently from 06e023a to 024e8d9 Compare May 5, 2024 05:33
@joeflack4 joeflack4 marked this pull request as ready for review May 5, 2024 05:33
@joeflack4 joeflack4 force-pushed the fhir-extension branch 2 times, most recently from 5d826c5 to feb14bd Compare May 5, 2024 05:39
Copy link
Collaborator

@matentzn matentzn left a comment

Choose a reason for hiding this comment

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

Nice changes! Some comments, but its close

src/sssom/writers.py Show resolved Hide resolved
src/sssom/writers.py Show resolved Hide resolved
src/sssom/writers.py Outdated Show resolved Hide resolved
src/sssom/writers.py Outdated Show resolved Hide resolved
src/sssom/writers.py Outdated Show resolved Hide resolved
tests/test_cli.py Show resolved Hide resolved
matentzn
matentzn previously approved these changes May 13, 2024
Copy link
Collaborator

@matentzn matentzn left a comment

Choose a reason for hiding this comment

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

I did not do a detailed code review, just looked out for breaking changes. If you want me to merge and run a release, ping me on slack.

src/sssom/writers.py Outdated Show resolved Hide resolved
- Added: schema/fhir_json.csv: For automation of conversion by utilizing CSV export from curated GoogleSheet.
- Update: Comments: Added state of mappings between SSSOM and FHIR ConceptMap
- Update: Minor codestyle updates, comments, todo's
- Delete: Unused schema/fhir_json.csv. Mappings will continue to be done via pure Python
- Bugfix: extension.ValueString -> extension.valueString
- Delete: Comments about spec field mappings
- Refactored 3 JSON writer functions to a state that they were probably intended to be in: 1 json function with branching logic depending on 'serialization' param.
- Add: _test_to_fhir_json()
- Update: test_write_sssom_fhir(): More assertions
- Update: Fixed some typos
- Update: model mappings: exporter: title --> title (was previously name --> title)
- Update: Drepecated these functions instead of deleting: write_fhir_json(), write_ontoportal_json()
@matentzn matentzn merged commit 91d08b8 into master May 14, 2024
6 checks passed
@matentzn matentzn deleted the fhir-extension branch May 14, 2024 10:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants