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

Support multiple dicom versions #83

Merged

Conversation

smasuda
Copy link
Contributor

@smasuda smasuda commented May 14, 2024

The current anonymizer uses the basic profile of attributes of DICOM 2013 which is different from the latest version of DICOM, 2024b.

To keep backward compatiblity while supporting for future DICOM spec changes, I wonder if this set of attributes can be switchable by supplying a factory method to anonymize_dataset method.

@pchoisel
Copy link
Collaborator

Hi !
Thank you for your contribution
The standard we currently use is Nov. 20, 2023, we updated it in #60
I'm totally fine with updating the standard we use to 2024b, but I don't know about having a selector for different versions.

Do you have a use case where you'd want to switch between the latest standard and one from several years ago ?

@smasuda
Copy link
Contributor Author

smasuda commented May 22, 2024

Hi @pchoisel !

Thanks for your review and comment!
Correct me if I'm wrong, - the scraper script was indeed updated to use the latest, yet the dicomfields.py, which is used as default, has not been updated and it is still of 2013 as in the comment.

The selector would be useful because of..:

  1. Backward compatibility. Because the default tags are of 2013 provided by dicomfields.py, you might not be able to switch to the latest without upsetting the existing users, though personally I'm fine to switch to the latest by default.
  2. Enabling to introduce a totally different set of tags in a simpler manner. For example, currently if I want to define my own set of tags, first I need to load "ALL_TAGS" and mark them keep, then I can introduce my tags. By the selector, this "keep" part can be eliminated by defining my own selector which only concerns the tags I'm interested in.

@smasuda smasuda force-pushed the SUPPORT_MULTIPLE_DICOM_VERSIONS branch from d45acc0 to 6987755 Compare June 21, 2024 06:08
@smasuda smasuda marked this pull request as draft June 21, 2024 06:13
@smasuda smasuda force-pushed the SUPPORT_MULTIPLE_DICOM_VERSIONS branch 2 times, most recently from 585b72c to a4b82e8 Compare June 21, 2024 06:53
@smasuda smasuda marked this pull request as ready for review June 21, 2024 07:11
@pchoisel
Copy link
Collaborator

pchoisel commented Jul 1, 2024

Hi @smasuda and sorry for the delay,

The comment you are mentioning is wrong, our DICOM field database has been updated with 2023's version.
However, we agree that a mechanism allowing the selection of a specific DICOM field anonymization database would be profitable.
I'll do a basic review then I'll merge this.

dicomanonymizer/dicomfields_2024b.py Outdated Show resolved Hide resolved
dicomanonymizer/dicomfields_selector.py Outdated Show resolved Hide resolved
dicomanonymizer/dicomfields_selector.py Outdated Show resolved Hide resolved
@smasuda
Copy link
Contributor Author

smasuda commented Jul 17, 2024

@pchoisel thanks for the review! I'll work on them.
Btw, are you sure that the current dicomfields are of 2023, not of 2013?
The comment of the file says that it is from 2013

@smasuda smasuda force-pushed the SUPPORT_MULTIPLE_DICOM_VERSIONS branch from b0e4e98 to a827067 Compare July 24, 2024 00:38
@smasuda
Copy link
Contributor Author

smasuda commented Jul 24, 2024

@pchoisel I've updated the PR based on your review comments above, except for leaving the current version as 2013, until we agree on this topic: #83 (comment).
Once it is cleared, I'm happy to make it as 2023 instead of 2013.

@smasuda smasuda requested a review from pchoisel July 24, 2024 00:57
@pchoisel
Copy link
Collaborator

pchoisel commented Jul 26, 2024

Hi @smasuda, really sorry for my lack of reactivity these past weeks.
I almost certain we are using the 2023 version of the DICOM standard. If you look at the 2014a standard, (table E1-1), you can see 271 fields to anonymize while there are 610 in the 2023e standard, like in the file dicomfields.py.

I'm sure we updated the dicomfields.py file with the scrapper tool, but we forgot to update the comment in line 3.

Apart from this point, it looks good to me !

@smasuda
Copy link
Contributor Author

smasuda commented Jul 29, 2024

@pchoisel thanks for your confirmation! I've updated all the reference of the version year 2013 to be 2023.

@pchoisel
Copy link
Collaborator

Thank you,

I quickly tested the software, there seem to be a small parameter issue when calling anonymize_dataset from anonymize_dicom_file in simpledicomanonymizer.py line 390.
The parameter extra_anonymization_rules is treated as base_rules_gen.

Maybe base_rules_gen should be moved further in the parameter list of anonymize_dataset ?

@smasuda
Copy link
Contributor Author

smasuda commented Jul 30, 2024

@pchoisel you're right - thanks for spotting it.
As suggested I have further moved the paramter to anonymize_dataset.
Also, I've put the new argument(base_rule_gen) at the end of the existing argument list for backward compatibility.

@pchoisel
Copy link
Collaborator

Looks good to me, thanks !

@pchoisel pchoisel merged commit c257ef7 into KitwareMedical:master Jul 31, 2024
4 checks passed
@smasuda
Copy link
Contributor Author

smasuda commented Jul 31, 2024

super. thanks for your help!

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