Update get_csa_header to return none on CSAReadError#501
Update get_csa_header to return none on CSAReadError#501parneshraniga wants to merge 4 commits intonipy:masterfrom
Conversation
Some non-MRI dicom datasets have just strings in CSA Header tags. This causes the read on line 68 to throw an error because the string is not a CSA header. Unfortunately this throws out other packages such as dcmstack which use nibabel. The proposed fix returns none if a read error is encountered when such strings are parsed. It is possible such strings may pass the initial check for 0 < n_tags <=128 which throws the CSAReadError but fail later in which case the except block can be be modified to handle those exceptions as well.
Current coverage is 94.02% (diff: 100%)@@ master #501 diff @@
==========================================
Files 166 166
Lines 21832 21832
Methods 0 0
Messages 0 0
Branches 2325 2325
==========================================
Hits 20527 20527
Misses 875 875
Partials 430 430
|
|
Thanks for this - have you got an example DICOM you can share, that we can use to test? Is there anything about the string that allows us to detect its presence in a more specific way than an error in the csa read routine? |
|
Thus far it seems to fail for Siemens PET data. I will check to see if the data can be shared or is already available and let you know. On the two datasets that I checked, there wasn't anything in particular that stood out but I will go through the data I have to see what I can find. |
|
You can delete the data in the DICOM file if you want, and replace it with zeros, so the file compresses well, and doesn't reveal private brain data. |
|
@parneshraniga Can you merge master or rebase to fix Travis tests? |
|
Done. |
|
Great! Thanks. Any luck getting an anonymized DICOM that was failing before? |
|
Yes I can grab a couple of files, anonymise them and clear the data out. Whats the best way to forward them to you? |
|
Any place we can grab them from. Google drive or dropbox are common. If they're small, they can go in nibabel/tests/data in a new commit; if they're big, see http://nipy.org/nibabel/devel/add_test_data.html. |
|
Guys - now is an excellent time to get this one finished up - I have lots of free time at the moment to work on this. |
|
@parneshraniga - would you mind sharing those files? That would be very helpful. |
|
@parneshraniga - anything we can do to help? Can you give us access to the unmodified files? We can anonymize them for you. |
|
@parneshraniga - another reminder in hope that we can get this one in. |
|
@parneshraniga Do you have any time to help get this one finished? |
|
Hi @parneshraniga, is there any chance you still have access to files that can reproduce this issue? |
6 participants
Some non-MRI dicom datasets have just strings in CSA Header tags e.g Siemens PET dicoms . This causes the read on line 68 to throw an error because the string is not a CSA header. Unfortunately this throws out other packages such as dcmstack which use nibabel. The proposed fix returns none if a read error is encountered when such strings are parsed. It is possible such strings may pass the initial check for 0 < n_tags <=128 which throws the CSAReadError but fail later in which case the except block can be be modified to handle those exceptions as well.