ENH: Support for Philips DICOMs w/ derived volume #727

mgxd · 2019-02-20T21:31:19Z

Closes #693

Currently, if a Philips DICOM includes a derived volume, MultiframeWrapper blows up due to incorrect image_shape. This PR adds the following:

if DICOM has MR Diffusion Sequence (0018, 9117), iter through frames and discard any with type derived
~~hidden attribute _nframes to track if NumberOfFrames has changed~~
4D diffusion DICOM with tracer volume for testing (thanks to @bmelo)

I also noticed get_data() output is incorrect, but haven't addressed that (any ideas regarding that would be appreciated!) My main motivation for this was to fix nipy/heudiconv#275

Relevant material https://github.com/rordenlab/dcm2niix/blob/d04ba75c8b098240d0efef67379947ff75bc1d36/Philips/README.md#derived-parametric-maps-stored-with-raw-diffusion-data

* if DICOM has MR Diffusion Sequence (0018, 9117), discard any derived frames * out correct final shape * TODO: fix get_data() repr

nibabel/pydicom_compat.py

…ching

coveralls · 2019-02-21T00:20:18Z

Coverage decreased (-0.0002%) to 91.916% when pulling c59b30b on mgxd:fix/trace into ad0b13f on nipy:master.

effigies

Assume you're already looking into the test failures. Let me know if you want me to look at them too.

nibabel/pydicom_compat.py

nibabel/nicom/dicomwrappers.py

effigies · 2019-02-21T15:58:33Z

nibabel/nicom/dicomwrappers.py

        # More than 3 dimensions
        ns_unique = [len(np.unique(row)) for row in self._frame_indices.T]
+        if len(ns_unique) == 3:
+            # derived volume is included
+            ns_unique.pop(1)


I don't really follow this guard. Can you elaborate?

I'm not sure this addresses the problem properly / gracefully, but I'm unsure how to proceed. I've come across a derived image included in the diffusion DICOM, which is currently not properly handled. I thought this meant 5D data wasn't yet handled, but in fact there is a test specifically for this introduced in 3f04e3c (maybe @matthew-brett can share some wisdom)

nibabel/nibabel/nicom/tests/test_dicomwrappers.py

Lines 656 to 682 in ad0b13f

# 5D!

dim_idxs = [

[1, 4, 2, 1],

[1, 2, 2, 1],

[1, 3, 2, 1],

[1, 1, 2, 1],

[1, 4, 2, 2],

[1, 2, 2, 2],

[1, 3, 2, 2],

[1, 1, 2, 2],

[1, 4, 1, 1],

[1, 2, 1, 1],

[1, 3, 1, 1],

[1, 1, 1, 1],

[1, 4, 1, 2],

[1, 2, 1, 2],

[1, 3, 1, 2],

[1, 1, 1, 2]]

fake_mf.update(fake_shape_dependents(dim_idxs, sid_dim=0))

shape = (2, 3, 4, 2, 2)

data = np.arange(np.prod(shape)).reshape(shape)

sorted_data = data.reshape(shape[:2] + (-1,), order='F')

order = [11, 9, 10, 8, 3, 1, 2, 0,

15, 13, 14, 12, 7, 5, 6, 4]

sorted_data = sorted_data[..., np.argsort(order)]

fake_mf['pixel_array'] = np.rollaxis(sorted_data, 2)

assert_array_equal(MFW(fake_mf).get_data(), data * 2.0 - 1)

codecov-io · 2019-02-27T20:11:30Z

Codecov Report

Merging #727 into master will decrease coverage by 0.03%.
The diff coverage is 73.07%.

@@            Coverage Diff             @@
##           master     #727      +/-   ##
==========================================
- Coverage   89.55%   89.51%   -0.04%     
==========================================
  Files          93       93              
  Lines       11474    11497      +23     
  Branches     1992     1998       +6     
==========================================
+ Hits        10275    10291      +16     
- Misses        859      864       +5     
- Partials      340      342       +2

Impacted Files	Coverage Δ
nibabel/pydicom_compat.py	`100% <100%> (ø)`	⬆️
nibabel/nicom/dicomwrappers.py	`89.43% <68.18%> (-1.31%)`	⬇️

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 9430224...5181407. Read the comment docs.

bmelo · 2019-03-08T17:57:36Z

I tested the last commit with the same dataset I was working on when I opened the issue nipy/heudiconv#275. Now I can convert our images to BIDS without problems.

@mgxd, thank you so much!

effigies · 2019-03-13T01:47:04Z

@mgxd Is this still a WIP, or should I review?

mgxd · 2019-03-14T16:48:46Z

ready for a review

effigies

From what I can see this looks reasonable, tests pass, and @bmelo indicates that it resolves his problem over in HeuDiConv.

Inclined to merge, but I'll give it a couple days in case anybody with more DICOM experience wants to weigh in.

A couple comments.

nibabel/nicom/dicomwrappers.py

effigies · 2019-03-15T20:12:15Z

nibabel/nicom/dicomwrappers.py

@@ -522,6 +546,9 @@ def image_shape(self):
        if stackid_tag in dim_seq:
            stackid_dim_idx = dim_seq.index(stackid_tag)
            frame_indices = np.delete(frame_indices, stackid_dim_idx, axis=1)
+        if has_derived:
+            # derived volume is included
+            frame_indices = np.delete(frame_indices, 1, axis=1)


It is always the second entry?

Based off my limited testing yes - however we should definitely keep an eye on this

Can we use either a derived index or make an assertion that will fail loudly when the assumption is violated?

effigies · 2019-03-18T12:51:35Z

@mgxd If you have a few minutes, I think we can probably finish this up today. Just a question and a minor cleanup.

effigies · 2019-03-19T12:01:48Z

LGTM. Merging.

effigies · 2019-03-19T12:15:11Z

Ugh, saw after merging that your test file is 40MB. Had to force-push master back to pre-merge state. Can you find/produce a smaller version of the file, and open a new PR?

effigies · 2019-04-25T14:33:39Z

@mgxd Just a bump on this. I believe we were looking for a (possibly truncated) file with a compressed data frame.

effigies · 2019-07-12T12:25:03Z

Hi @mgxd, any news here?

mgxd · 2019-07-12T20:35:47Z

I'll see if I can get to it this weekend

effigies · 2019-08-21T13:36:25Z

Hi @mgxd. Sorry to take so long to think of this, but what if we just create a nibabel-data submodule? I can do it, if you let me know the license the DICOM can be released under.

mgxd · 2019-08-21T13:41:00Z

That sounds like a good idea.

@bmelo any restriction with the provided DICOM?

bmelo · 2019-08-21T13:46:52Z

No, you can use it without restrictions.

effigies · 2019-08-21T13:53:42Z

Okay, I published under CC0: https://github.com/effigies/nitest-dicom

@bmelo, please let me know if you have any preference for being credited.

bmelo · 2019-08-21T14:27:50Z

I have no preference. Thank you!

effigies · 2019-08-21T14:30:07Z

Cool. Feel free to open a PR to add your affiliation, or any provenance of the file here: https://github.com/effigies/nitest-dicom/blob/master/CREDITS

Thanks for providing this test file.

enh+wip: support for philips dcm w/ derived volume

3a985f5

* if DICOM has MR Diffusion Sequence (0018, 9117), discard any derived frames * out correct final shape * TODO: fix get_data() repr

effigies reviewed Feb 20, 2019

View reviewed changes

nibabel/pydicom_compat.py Outdated Show resolved Hide resolved

mgxd added 3 commits February 20, 2019 16:47

fix: (py)dicom import

bbd420a

fix: set missing import to None, ensure first frame exists before fet…

b7ef629

…ching

fix: skip if frame does not have get attribute

6ec7b0a

effigies reviewed Feb 21, 2019

View reviewed changes

mgxd added 2 commits February 27, 2019 15:10

sty: import specifics separately

039d4f0

fix: allow non-diffusion 5D

2af8950

fix: remove unnecessary check

c59b30b

rf: better error handling, reflect change on frame_indices

f95d716

mgxd changed the title ~~WIP+ENH: Support for Philips DICOMs w/ derived volume~~ ENH: Support for Philips DICOMs w/ derived volume Mar 14, 2019

add: warn if frames were altered

469f623

effigies self-assigned this Mar 14, 2019

effigies mentioned this pull request Mar 15, 2019

REL: 2.4.0 #740

Merged

20 tasks

effigies approved these changes Mar 15, 2019

View reviewed changes

mgxd added 2 commits March 18, 2019 11:04

Merge branch 'master' of https://github.com/nipy/nibabel into fix/trace

5409f50

rf: condense frame checking try block

dfd93e1

effigies added this to the 2.4.0 milestone Mar 18, 2019

fix: ensure derived index is removed

5181407

effigies merged commit 7faf8dd into nipy:master Mar 19, 2019

effigies mentioned this pull request Apr 25, 2019

appropriate reading/derivation of bvalues and bvectors in the dicom readers #152

Closed

effigies mentioned this pull request Aug 21, 2019

FIX: Correctly handle Philips DICOMs w/ derived volume #795

Merged

mgxd deleted the fix/trace branch February 8, 2024 14:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ENH: Support for Philips DICOMs w/ derived volume #727

ENH: Support for Philips DICOMs w/ derived volume #727

mgxd commented Feb 20, 2019 •

edited

Loading

coveralls commented Feb 21, 2019 •

edited

Loading

effigies left a comment

effigies Feb 21, 2019

mgxd Feb 27, 2019

codecov-io commented Feb 27, 2019 •

edited

Loading

bmelo commented Mar 8, 2019

effigies commented Mar 13, 2019

mgxd commented Mar 14, 2019

effigies left a comment

effigies Mar 15, 2019

mgxd Mar 18, 2019

effigies Mar 18, 2019

effigies commented Mar 18, 2019

effigies commented Mar 19, 2019

effigies commented Mar 19, 2019

effigies commented Apr 25, 2019

effigies commented Jul 12, 2019

mgxd commented Jul 12, 2019

effigies commented Aug 21, 2019 •

edited

Loading

mgxd commented Aug 21, 2019

bmelo commented Aug 21, 2019

effigies commented Aug 21, 2019

bmelo commented Aug 21, 2019

effigies commented Aug 21, 2019

	# 5D!
	dim_idxs = [
	[1, 4, 2, 1],
	[1, 2, 2, 1],
	[1, 3, 2, 1],
	[1, 1, 2, 1],
	[1, 4, 2, 2],
	[1, 2, 2, 2],
	[1, 3, 2, 2],
	[1, 1, 2, 2],
	[1, 4, 1, 1],
	[1, 2, 1, 1],
	[1, 3, 1, 1],
	[1, 1, 1, 1],
	[1, 4, 1, 2],
	[1, 2, 1, 2],
	[1, 3, 1, 2],
	[1, 1, 1, 2]]
	fake_mf.update(fake_shape_dependents(dim_idxs, sid_dim=0))
	shape = (2, 3, 4, 2, 2)
	data = np.arange(np.prod(shape)).reshape(shape)
	sorted_data = data.reshape(shape[:2] + (-1,), order='F')
	order = [11, 9, 10, 8, 3, 1, 2, 0,
	15, 13, 14, 12, 7, 5, 6, 4]
	sorted_data = sorted_data[..., np.argsort(order)]
	fake_mf['pixel_array'] = np.rollaxis(sorted_data, 2)
	assert_array_equal(MFW(fake_mf).get_data(), data * 2.0 - 1)

ENH: Support for Philips DICOMs w/ derived volume #727

ENH: Support for Philips DICOMs w/ derived volume #727

Conversation

mgxd commented Feb 20, 2019 • edited Loading

coveralls commented Feb 21, 2019 • edited Loading

effigies left a comment

Choose a reason for hiding this comment

effigies Feb 21, 2019

Choose a reason for hiding this comment

mgxd Feb 27, 2019

Choose a reason for hiding this comment

codecov-io commented Feb 27, 2019 • edited Loading

Codecov Report

bmelo commented Mar 8, 2019

effigies commented Mar 13, 2019

mgxd commented Mar 14, 2019

effigies left a comment

Choose a reason for hiding this comment

effigies Mar 15, 2019

Choose a reason for hiding this comment

mgxd Mar 18, 2019

Choose a reason for hiding this comment

effigies Mar 18, 2019

Choose a reason for hiding this comment

effigies commented Mar 18, 2019

effigies commented Mar 19, 2019

effigies commented Mar 19, 2019

effigies commented Apr 25, 2019

effigies commented Jul 12, 2019

mgxd commented Jul 12, 2019

effigies commented Aug 21, 2019 • edited Loading

mgxd commented Aug 21, 2019

bmelo commented Aug 21, 2019

effigies commented Aug 21, 2019

bmelo commented Aug 21, 2019

effigies commented Aug 21, 2019

mgxd commented Feb 20, 2019 •

edited

Loading

coveralls commented Feb 21, 2019 •

edited

Loading

codecov-io commented Feb 27, 2019 •

edited

Loading

effigies commented Aug 21, 2019 •

edited

Loading