BF: Remove assumption about StackID index #439
Conversation
…he frame indices for multiframe DICOM
Conflicts: nibabel/nicom/tests/test_dicomwrappers.py
|
Thanks a lot for this. Is there anything in the DICOM standard docs that you could copy / paste into the docstring here to explain the algorithm? |
|
Generally this looks good and well-written, but I don't know the part of the standard that this is implementing - can you give me some pointers to help review? Brendan - do you have time to take a look? |
|
I am currently inserting into the docstring a description of the algorithm and references to the DICOM standard. The relevant parts of Supplement 49 of the DICOM standard are:
|
|
Thanks - that will be very helpful. |
… the Multi-frame DICOM wrapper
|
I think this is the definitive source for the supplement : ftp://medical.nema.org/medical/dicom/final/sup49_ft.pdf - is that right? But - looking at the standard, it seems the supplement has already been included, e.g. |
| # the pointer pair (StackID Dicom-tag, FrameContentSequence Dicom-tag) | ||
| # that indicates that the corresponding axis in the DimensionIndexValues | ||
| # attribute refers to the StackID | ||
| stack_id_dim_pointer = ((0x20, 0x9056), (0x20, 0x9111)) |
There was a problem hiding this comment.
Maybe get these programmatically to make it more readable? As in something like:
from nibabel.pydicom_compat import pydicom as pd
stack_id_dim_pointer = pd.datadict.tag_for_name('StackID'), pd.datatict.tag_for_name('FrameContentSequence')
There was a problem hiding this comment.
Do we need to check the second "FrameContentSequence" tag? What if the first Tag is StackID and the second is something else? Is this an error in the DICOM?
There was a problem hiding this comment.
I will update the references to point to the current standard. I have been working off of the 2015c version of the standard that did not include the supplement.
I agree that using tag_for_name is preferable. I will rewrite like how you suggested. This brings up another thought that occurred to me: do you think that it would be beneficial to have a method/attribute that returned/stored the DICOM attribute name that each axis of the data corresponds to? A user could use the "DimensionIndexSequence" to determine these on their own, but doing it in such a manner could be error prone for them since we are removing the "StackID" axis.
I believe you are correct that the "FrameContentSequence" tag is not needed. It does appear that it would be an error in the DICOM (C.7.6.17.1 Dimension Indices) if the "FunctionalGroupPointer" was something other than the "FrameContentSequence" tag for "StackID". However, it does not appear that the DICOM standard guarantees that each axis will have a "FunctionalGroupPointer" attribute when one is not needed for that axis (i.e. the dimension attribute is not nested). If this was the case for an axis, then how I have it coded up now will not work. So i am going rewrite the code to just rely on the "DimensionIndexPointer".
There was a problem hiding this comment.
Thanks for the feedback. It would be excellent to have a way of returning an attribute name per axis. I'm hoping to get some time in the next few months to add named axes to nibabel image objects, and it would be very helpful to be able to get names from dicom image axes.
There was a problem hiding this comment.
For the reference, given that the supplement has just been included, maybe add a reference to the supplement and the most recent version of the standard.
|
Thanks for this - the code is well written and tested, good to read. |
MRG: test fixes and doc updates for multiframe code Fixes to some new test failures at : https://travis-ci.org/matthew-brett/nibabel/builds/121846880 More links to sections and diagrams on multi-frame / enhanced MR image IOP. Follow-up to PR #439.
In determining the shape of a multi-frame DICOM, the first value in the DimensionIndexValues was taken to be the StackID. The DICOM standard does not appear to guarantee this to always be true. Replaced the assumption by using the DimensionIndexSequence to determine the position of the StackID in the DimensionIndexValues.