KitwareMedical / dicom-anonymizer

Tool to anonymize DICOM files according to the DICOM standard
BSD 3-Clause "New" or "Revised" License
104 stars 47 forks source link

Support multiple dicom versions #83

Closed smasuda closed 3 months ago

smasuda commented 5 months ago

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 commented 5 months ago

Hi ! Thank you for your contribution The standard we currently use is Nov. 20, 2023, we updated it in https://github.com/KitwareMedical/dicom-anonymizer/pull/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 commented 5 months ago

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.
pchoisel commented 4 months ago

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.

smasuda commented 3 months ago

@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 commented 3 months ago

@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: https://github.com/KitwareMedical/dicom-anonymizer/pull/83#issuecomment-2232121976. Once it is cleared, I'm happy to make it as 2023 instead of 2013.

pchoisel commented 3 months ago

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 commented 3 months ago

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

pchoisel commented 3 months ago

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 commented 3 months ago

@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 commented 3 months ago

Looks good to me, thanks !

smasuda commented 3 months ago

super. thanks for your help!