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

anonymize_dataset fails if dataset contains RawDataElement #85

Open
GuillaumeDehaene opened this issue May 27, 2024 · 5 comments
Open

Comments

@GuillaumeDehaene
Copy link

Hello,

Thank you for this great project.

While using it in our codebase, I have found the following issue.

anonymize_dataset fails if dataset contains RawDataElement

anonymize_dataset fails if the dataset contains RawDataElement:

Proposed solution

I would be happy to create a PR to fix this issue here too.

I see two possibilities:

  1. Sanitize dataset as part of anonymize_dataset:
    • iterate over the dataset and replace RawDataElements,
    • continue with the current code of anonymize_dataset.
  2. Modify current replace_element code to handle the RawDataElement case.

I'm partial to solution 1.:
- it's simple and easy to understand and review.
- it makes it easier for the user to add custom-rules since they are able to assume that all elements are RawDataElement, and they can use the simpler: element.value = new_value syntax.
- however, it also means that input dataset are walked-through twice. I feel that this price is worth paying.

Best
Guillaume

@pchoisel
Copy link
Collaborator

Hi @GuillaumeDehaene, please forgive me for the lack of reactivity, I now have a bit of time to work on this project.

I understand the problem but I'm wondering how can this problem happen ? Do those RawDataElement come directly from DICOM files or are you anonymizing a custom dataset ?

To fix this issue, instead of parsing the dataset twice to sanitize it, couldn't we just drop the tag if we detect that it is a RawDataElement ? This could be done in dicomanonymizer/simpledicomanonymizer.py::anonymize_dataset()

@GuillaumeDehaene
Copy link
Author

Hello @pchoisel

No worries. Thank you for maintaining this project.

So this was coming from a real DICOM dataset, but I'm not in contact with the person who generated it so I don't have any info regarding which software, which kind of data, etc.
I also was unable to figure out what this RawDataElement corresponds to in pydicom 🤷

I guess we could also throw out any raw data but that feels very rough, doesn't it?
Double-parsing the data is suboptimal but it is also simple. Are you so worried about the performance hit?

Anyway, I've made my case and I think those are the options.
If you let me know which option you prefer, I'll write a PR for it and we can discuss how to proceed further on that basis.

Thank you again for your work on this project.
Best regards
Guillaume

@pchoisel
Copy link
Collaborator

pchoisel commented Aug 5, 2024

Hi @GuillaumeDehaene,

I definitely don't want to parse the dataset twice. That's mainly because people (at least us) use this software in automated processes that anonymize lots of DICOM files.

I'm fine with not dropping the RawDataElement. Have you checked the function DataElement_from_raw ? Maybe we could use it to handle the RawDataElement. If you want to keep those tags as RawDataElement, maybe you can re-transform them before writing the output file ?

What do you think ?

@GuillaumeDehaene
Copy link
Author

GuillaumeDehaene commented Aug 14, 2024 via email

@pchoisel
Copy link
Collaborator

That sounds perfect !

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

No branches or pull requests

2 participants