Skip to content

Conversation

hbraunDSP
Copy link
Contributor

@hbraunDSP hbraunDSP commented Sep 24, 2019

Workaround for #812.
dicomwrappers.py will catch and recover from a CSAReadError in csarereader.py, setting csa=None and giving a warning. This allows use on some dicoms with CSA headers that are unreadable by nibabel, either because they are corrupted or not supported yet.

@pep8speaks
Copy link

pep8speaks commented Sep 24, 2019

Hello @hbraunDSP, Thank you for updating!

Cheers! There are no style issues detected in this Pull Request. 🍻 To test for issues locally, pip install flake8 and then run flake8 nibabel.

Comment last updated at 2019-11-11 15:57:08 UTC

@effigies
Copy link
Member

@hbraunDSP Is this still a WIP? If you can find a test file, that'd be great. Feel free to submit to https://github.com/effigies/nitest-dicom/.

@hbraunDSP
Copy link
Contributor Author

It should be good to go...I'll be sending a file for testing soon. Thanks for the reminder!

@hbraunDSP hbraunDSP changed the title WIP: Ignore unreadable siemens private headers Ignore unreadable siemens private headers Oct 30, 2019
@effigies
Copy link
Member

effigies commented Nov 5, 2019

We'll need to update nitest-dicom in https://github.com/nipy/nibabel/tree/master/nibabel-data:

git submodule init
git submodule sync
git submodule update
git -C nibabel-data/nitest-dicom fetch
git -C nibabel-data/nitest-dicom checkout origin/master
git add nibabel-data/nitest-dicom

Then we'll need to add the new file as follows:

DATA_FILE_4D_DERIVED = pjoin(get_nibabel_data(), 'nitest-dicom',
'4d_multiframe_with_derived.dcm')

And test it similar to the following:

@dicom_test
@needs_nibabel_data('nitest-dicom')
def test_data_derived_shape(self):
# Test 4D diffusion data with an additional trace volume included
# Excludes the trace volume and generates the correct shape
dw = didw.wrapper_from_file(DATA_FILE_4D_DERIVED)
assert_equal(dw.image_shape, (96, 96, 60, 33))

@codecov
Copy link

codecov bot commented Nov 5, 2019

Codecov Report

Merging #818 into master will increase coverage by 0.26%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #818      +/-   ##
==========================================
+ Coverage    90.1%   90.36%   +0.26%     
==========================================
  Files          96       96              
  Lines       11911    12196     +285     
  Branches     2125     2136      +11     
==========================================
+ Hits        10732    11021     +289     
+ Misses        834      832       -2     
+ Partials      345      343       -2
Impacted Files Coverage Δ
nibabel/nicom/csareader.py 87.4% <ø> (+2.96%) ⬆️
nibabel/nicom/dicomwrappers.py 90.9% <100%> (+0.65%) ⬆️
nibabel/funcs.py 80.28% <0%> (-0.28%) ⬇️
nibabel/gifti/giftiio.py 100% <0%> (ø) ⬆️
nibabel/keywordonly.py 100% <0%> (ø) ⬆️
nibabel/streamlines/tractogram_file.py 100% <0%> (ø) ⬆️
nibabel/arrayproxy.py 100% <0%> (ø) ⬆️
nibabel/affines.py 100% <0%> (ø) ⬆️
nibabel/imageclasses.py 100% <0%> (ø) ⬆️
nibabel/streamlines/array_sequence.py 100% <0%> (ø) ⬆️
... and 36 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1d91b72...702515e. Read the comment docs.

@effigies
Copy link
Member

@hbraunDSP Just a bump on these tests. If you don't have time, I can make a quick PR to your branch.

@effigies
Copy link
Member

See hbraunDSP#2.

@hbraunDSP
Copy link
Contributor Author

hbraunDSP commented Nov 11, 2019

I can't really look at this until Wednesday, sorry. I just merged your PR after a brief glance.

TEST: Basic tests for CT DICOM image
@effigies
Copy link
Member

I think this is good as-is, and we're now getting full diff coverage (thanks!). Feel free to open a new PR if there's anything else you'd like to update...

@effigies effigies merged commit b2a88b8 into nipy:master Nov 11, 2019
@effigies effigies added this to the 3.0.0 milestone Nov 15, 2019
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.

3 participants