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

Embed project name as tag in DICOM image #329

Merged
merged 43 commits into from
Mar 14, 2024

Conversation

jeremyestein
Copy link
Contributor

@jeremyestein jeremyestein commented Mar 1, 2024

Fix #296

TODO:

  • Change group id from 0x000B to 000D?

Also add to system test to check presence of private tag. Think this will
have to be moved to check in a different place but has value for
checking other pseudonymised tags?
@jeremyestein jeremyestein changed the title WIP - Add a private tag but the API call to modify it doesn't work [force-system-test] Embed project name as tag in image [force-system-test] Mar 4, 2024
due to never changing this via the API during anonymisation.
@jeremyestein jeremyestein marked this pull request as ready for review March 7, 2024 20:16
@jeremyestein jeremyestein changed the title Embed project name as tag in image [force-system-test] Embed project name as tag in DICOM image Mar 7, 2024
@jeremyestein jeremyestein requested a review from a team March 7, 2024 20:44
Copy link
Member

@milanmlft milanmlft left a comment

Choose a reason for hiding this comment

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

Looking good! 🚀

test/system_test.py Outdated Show resolved Hide resolved
Copy link
Contributor

@stefpiatek stefpiatek left a comment

Choose a reason for hiding this comment

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

We also need to use the project name from within orthanc anon (IIRC code where we do this is in pixl_dcmd), instead of querying the database to get the project name (see the checklist in the issue)

orthanc/orthanc-raw/plugin/pixl.py Show resolved Hide resolved
pixl_imaging/src/pixl_imaging/_orthanc.py Outdated Show resolved Hide resolved
pixl_imaging/tests/docker-compose.yml Outdated Show resolved Hide resolved
test/system_test.py Outdated Show resolved Hide resolved
@jeremyestein
Copy link
Contributor Author

We also need to use the project name from within orthanc anon (IIRC code where we do this is in pixl_dcmd), instead of querying the database to get the project name (see the checklist in the issue)

This is fixed now.

However, it's not ideal that the private tag info (0x000B, 0x01, "UCLH PIXL", "UCLHPIXLProjectName") is copied over so many locations in python, json, and yaml.

The python locations could surely be easily reduced to one place, but the others may be a bit more fiddly.

One approach would be to have a single source of truth for our private tags in a json file, and use jsonnet to generate the various json config files from this. Python can trivially load the json file. For the yaml I'm not sure - yaml is a superset of JSON so maybe when loading the tag scheme we can concatenate it with the json file? Or just switch the tag schemes to json(net)?

Anyway, this should go in a separate issue. But I'll try and fix the python usages in this PR.

…tag. There is still

duplication in json and yaml files unfortunately.
@jeremyestein jeremyestein requested a review from stefpiatek March 11, 2024 18:16
Copy link
Contributor

@stefpiatek stefpiatek left a comment

Choose a reason for hiding this comment

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

Looks good, thanks Jeremy!

Can we also remove the use of get_project_slug_from_hashid from orthanc-anon/pixl.py, instead querying the API for the project name using the resourceId?, then we can pass that to the ftp upload, and then we don't have to query for the project name in core.uploader._ftps and then remove the get_project_slug_from_hashid entirely

hashed_patient_id = _get_patient_id(resourceId)
project_slug = get_project_slug_from_hashid(hashed_patient_id)

orthanc/orthanc-raw/plugin/pixl.py Show resolved Hide resolved
@jeremyestein
Copy link
Contributor Author

Looks good, thanks Jeremy!

Can we also remove the use of get_project_slug_from_hashid from orthanc-anon/pixl.py, instead querying the API for the project name using the resourceId?, then we can pass that to the ftp upload, and then we don't have to query for the project name in core.uploader._ftps and then remove the get_project_slug_from_hashid entirely

hashed_patient_id = _get_patient_id(resourceId)
project_slug = get_project_slug_from_hashid(hashed_patient_id)

Looking at get_project_slug_from_hashid, as a side effect it checks whether the image has already been exported.
Do we want to keep a call to _query_existing_image for this reason? Or will the uploader service fix this better anyway, and we can live with re-uploads for the time being? I'm not sure in what situations we would encounter this condition.

    with PixlSession() as pixl_session, pixl_session.begin():
        existing_image = _query_existing_image(pixl_session, hashed_value)

        if existing_image.exported_at is not None:
            msg = "Image already exported"
            raise RuntimeError(msg)

…he local orthanc

API which we're calling anyway. orthanc anon does need the dictionary so
we can refer to the private tag by name in the API.
image has already been uploaded. Clarify the PatientID tag meaning.
Copy link
Contributor

@stefpiatek stefpiatek left a comment

Choose a reason for hiding this comment

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

Great, thanks for finishing this up Jeremy!

@jeremyestein jeremyestein merged commit edd9ad5 into main Mar 14, 2024
8 checks passed
@jeremyestein jeremyestein deleted the jeremy/custom-project-dicom-tag branch March 14, 2024 17:30
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.

Store project name in DICOM tags
5 participants