Skip to content

ENH: Add writer for Siemens CSA header #417

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

Merged
merged 2 commits into from
Sep 5, 2024
Merged

Conversation

moloney
Copy link
Contributor

@moloney moloney commented Mar 3, 2016

Allows us to take a parsed CSA header and convert it back into a
string. Useful for things like DICOM anonymization, or perhaps
round tripping DICOM -> Nifti -> DICOM.

@moloney
Copy link
Contributor Author

moloney commented Mar 3, 2016

This is another bite-sized chunk from #290.

I was tempted to make a csa module with the read/writer functionality and then setup some proxies in csareader for backwards compatibility, but I am not sure it is worth the hassle?

It would also be nice to use ElemDict objects (as specified in #416) for the parsed representation. Again, I guess we need to provide some backwards compatibility here.

@nibotmi
Copy link
Contributor

nibotmi commented Mar 6, 2016

☔ The latest upstream changes (presumably #393) made this pull request unmergeable. Please resolve the merge conflicts.

@matthew-brett
Copy link
Member

The csa representation as we wrote it originally is a bit kludgy, I think - if you're interested - it would be worth improving that with a new csa module, and depracting the csareaders module.

I don't think we have to worry as much as usual about backwards compatibility - the nicom module has always had a bit fat warning on it. So, avoid breakage if possible, but don't do excessive work to keep compatibility otherwise.

@matthew-brett
Copy link
Member

By the way - thanks very much for breaking out the change from the big PR - it's extremely helpful.

moloney added 2 commits March 23, 2020 19:57
Allows us to take a parsed CSA header and convert it back into a
string. Useful for things like DICOM anonymization, or perhaps
round tripping DICOM -> Nifti -> DICOM.
@codecov
Copy link

codecov bot commented Mar 24, 2020

Codecov Report

Attention: Patch coverage is 81.63265% with 9 lines in your changes missing coverage. Please review.

Project coverage is 91.66%. Comparing base (65d5fc6) to head (122a923).
Report is 1222 commits behind head on master.

Files with missing lines Patch % Lines
nibabel/nicom/csareader.py 81.63% 6 Missing and 3 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #417      +/-   ##
==========================================
- Coverage   91.70%   91.66%   -0.04%     
==========================================
  Files          96       96              
  Lines       12311    12360      +49     
  Branches     2173     2186      +13     
==========================================
+ Hits        11290    11330      +40     
- Misses        684      690       +6     
- Partials      337      340       +3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@moloney
Copy link
Contributor Author

moloney commented Aug 9, 2021

@ZviBaratz @matthew-brett @effigies

During our recent meeting, I forgot the original reason I wrote this was to support anonymization of Siemens mosaic/DWI Dicom files (not round-tripping back to DICOM). The standard way to anonymize DICOM removes all private headers, but then you lose the mosaic info and b-values/b-vecs. You can whitelist the CSA sub-header with this info, but at least some versions of the scanner software will use uninitialized memory for the CSA buffer and then leak PHI in the unused portions. So by parsing it and rewriting it with this code (which does zero out the memory) you can avoid this issue. So... it is pretty niche but still useful.

This also got me thinking, is dicom_parser going to make any attempt at supporting anonymization? Should we start a dicom_anon project?

@ZviBaratz
Copy link
Contributor

I haven't really given it much thought so far, but I definitely support the idea. Do you think a separate package will be better? I feel like it could probably be added to dicom_parser fairly easily, but it's possible I'm simply not aware of some anonymization edge-cases, such as the CSA thing.

@effigies
Copy link
Member

effigies commented Sep 5, 2024

I'm going to go ahead and merge this. Coverage is reasonable, and can be built off of. Sorry for taking so long to get back to this.

@effigies effigies merged commit 09d3f95 into nipy:master Sep 5, 2024
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.

5 participants