- Notifications
You must be signed in to change notification settings - Fork269
Update get_csa_header to return none on CSAReadError#501
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:master
Are you sure you want to change the base?
Uh oh!
There was an error while loading.Please reload this page.
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.
coveralls commentedJan 3, 2017
coveralls commentedJan 3, 2017
codecov-io commentedJan 4, 2017 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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
|
coveralls commentedJan 4, 2017
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? |
coveralls commentedJan 12, 2017
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 innibabel/tests/data in a new commit; if they're big, seehttp://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? |
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.