-
Notifications
You must be signed in to change notification settings - Fork 0
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
Use imaging UID from OMOP-ES extract #469
Conversation
…_dicom_from_scheme()` Avoids requiring `study_info` arg in `_anonymise_dicom_from_scheme()` and limiting its responsibilities
Avoids magic values that might cause confusion about where they're coming from.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #469 +/- ##
==========================================
- Coverage 84.11% 83.02% -1.10%
==========================================
Files 83 80 -3
Lines 3545 3352 -193
==========================================
- Hits 2982 2783 -199
- Misses 563 569 +6 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good, tests are catching some errors. UIDs are in omop es, so you can ask Baptiste to set off a pinpoint run again if you want to test on real data
7b092d5
to
fb5999b
Compare
fb5999b
to
41e8066
Compare
`sqlalchemy` actually does raise an error when `.one()` doesn't return anything.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking glorious thanks!
I wonder for pinpoint we could just delete all rows which haven't been exported and then run populate so the UIDs will exist in the ones its still trying to export. From thereon in, we wouldn't expect the UID to change so that should be fine
pixl_imaging/alembic/versions/83dcb3812628_add_study_uid_column_to_image_table.py
Outdated
Show resolved
Hide resolved
Co-authored-by: Stef Piatek <s.piatek@ucl.ac.uk>
Co-authored-by: Stef Piatek <s.piatek@ucl.ac.uk>
Sounds reasonable! |
Description
Related to https://github.com/UCLH-Foundry/the-rolling-skeleton/issues/161
I suspect this has to wait until https://github.com/UCLH-Foundry/the-rolling-skeleton/issues/161 is done and the OMOP-ES extracts actually contain the study UIDs.
Image
model and use for querying during anonymisationAlso cleaned up the
pixl_dcmd
tests and fixtures a bit, as I was getting confused where all the hardcoded values were coming from.Type of change
Please delete options accordingly to the description.
Suggested Checklist
main
branch.UCLH-Foundtry/arc-dev
squash and merge