ENH: Add Nifti1DicomExtension + test #296

kastman · 2015-02-12T21:59:40Z

Hi all,

Here's a thin wrapper class to read DICOM binary header information as encoded with the DICOM extended header code 2, instead of just providing the ugly & useless byte string. The extension code already written was quite helpful; thanks!

This depends on pydicom, but should fall back to a standard Nifti1Extension in the case of an ImportError. This is my first commit to nipy, so I'm not sure how you feel about provisional dependencies like this, but I think delegating the work to pydicom is the right approach.

Few things still to be worked out before you merge:

What do I have to do for CI to get this to either load or not load pydicom during testing?
This is hard-coded for little endian encodings right now - does anyone know a heuristic? Maybe @darcymason ?
This is written inside the Nifti1 format file, since the Nifti2 format seems to just delegate to Nifti1. Is that the right spot for it?
I'm not a great test-writer - could I improve on what's there?
I added import struct in the test to create DICOM byte-strings, but it looked out of place. Is there a better spot / method?
Is there a requirement that Nifti extensions should be divisible by 16 bytes? I saw that in another extension test, but am not aware of the requirement and couldn't find it in the docs. If so, I'll have to add some byte-padding code.

Hope this is helpful; let me know if there are any concerns about it! Cheers,
Erik

coveralls · 2015-02-12T22:10:14Z

Coverage decreased (-0.05%) to 94.27% when pulling 0d115dd on kastman:ENH-NiftiDicomExt into 96d474c on nipy:master.

matthew-brett · 2015-02-12T22:18:46Z

Thanks a lot for doing this.

Absolutely fine for the pydicom optional dependency - we already have that. Have a look at ./nibabel/nicom/tests/test_utils.py for an example of how to deal with the optional dependency.

Did you see: http://nipy.org/nibabel/devel/add_test_data.html for adding test data?

For the tests - the trick is to go through each function / method and ask yourself 'how would I know if this method was doing the wrong thing'. It is a rather tiring process, but it also proves very helpful in learning, at least in my experience.

kastman · 2015-02-13T02:43:36Z

I did see the page for adding test data, but I wasn't sure if it was appropriate to add another sample image (even a small one) for a test like this. Thanks for the tip on test_utils; that will work. Looks like I'm also failing on py3; I'll fix that as well.

matthew-brett · 2015-02-13T02:46:52Z

For a test image - if it is smaller than 50 compressed - sure - go ahead and add it to the main repo. Otherwise, a submodule would better. No problem for a very small submodule with only a few images.

kastman · 2015-02-13T02:51:49Z

Ironically, my actual use-case is a PET volume that's 1.6GB, so I'll definitely be making a new image for the test. See what I can do!

coveralls · 2015-02-13T21:28:36Z

Coverage decreased (-0.06%) to 94.26% when pulling 267304c on kastman:ENH-NiftiDicomExt into 96d474c on nipy:master.

coveralls · 2015-02-13T22:00:11Z

Coverage decreased (-0.01%) to 94.31% when pulling dbe3946 on kastman:ENH-NiftiDicomExt into 96d474c on nipy:master.

Also, fix _guess_implicit_VR method.

coveralls · 2015-02-14T11:56:01Z

Coverage decreased (-0.0%) to 94.32% when pulling b5b6550 on kastman:ENH-NiftiDicomExt into 96d474c on nipy:master.

* Remove redundant get size method (inherited) * Remove unnecessary super()

coveralls · 2015-02-16T01:37:29Z

Coverage decreased (-0.07%) to 94.26% when pulling 32fd4b1 on kastman:ENH-NiftiDicomExt into 96d474c on nipy:master.

coveralls · 2015-02-16T02:33:24Z

Coverage decreased (-0.06%) to 94.26% when pulling bf75e49 on kastman:ENH-NiftiDicomExt into 96d474c on nipy:master.

matthew-brett · 2015-02-16T19:29:04Z

Thanks for keeping up the work on this.

Are there any good docs on what the DICOM extension format should contain. I see this : http://nifti.nimh.nih.gov/nifti-1/documentation/nifti1fields/nifti1fields_pages/extension.html - which only says:

2 = DICOM format (i.e., attribute tags and values)

Do you know of anyone else writing these extensions?

matthew-brett · 2015-02-16T19:52:31Z

nibabel/nifti1.py

+            is_implicit_VR = False
+            is_little_endian = False
+        elif transfer_syntax == dicom.UID.DeflatedExplicitVRLittleEndian:
+            zipped = fileobj.read()


Have you got pyflakes or similar running in your editor? Pyflakes tells me that fileobj and zlib are not defined - is this piece of code tested?

You're right - I took this directly from pydicom, although it wasn't abstracted in a method I could call from nibabel. The only transfer syntax tested here is ImplicitVRLittleEndian, not the zipped or big endian ones. I'll add tests and fix this.

I haven't been using Pyflakes, but just grabbed a bundle to enable it. Thanks for the suggestion!

Pyflakes is huge - it really helps picking up this kind of thing.

kastman · 2015-02-16T20:00:20Z

I've only ever needed this once, for PMOD, a closed-source tool to model PET time activity curves. They write dicom tags to indicate the start time and duration of PET frames in a 4d nifti; the closet I could find for documentation is a listing of what they support for nifti. I only noticed they were using the DICOM extension when I saw they were able to read the timing from PMOD-created files and started looking around.

The tags are written with an explicit VR syntax without any metadata. An example header looks like this:

Nifti1Extension('dicom', 
  (0054, 1001) Units                                     CS: 'Bq/ml'
  (0055, 0010) Private Creator                     LO: 'PMOD_1'
  (0055, 1001) [Frame Start Times Vector]  FD: [0.0, 30.0, 60.0, ..., 13720.0, 14320.0]
  (0055, 1004) [Frame Durations (ms) Vector] FD: [30.0, 30.0, 30.0, ...,600.0, 600.0]')

Everyone else I know that wants DICOM info in NiFtI files uses @moloney 's dcmstack xml encoding; and I see there's already effort for that w/ nibabel (e.g. #232, #290). However, I figured that since I was implementing this, I might as well put in a way of storing full DICOM datasets in case someone wanted to use it.

matthew-brett · 2015-02-16T20:04:20Z

Well - I guess we can define the standard format. So would that be the explicit VR syntax? Maybe with little-endian byte order? Do the PMOD files always have little-endian order? And then try our best to read DICOM extensions that are written with implicit VR and big-endian.

matthew-brett · 2015-02-16T20:07:50Z

nibabel/nifti1.py

@@ -380,6 +380,113 @@ def write_to(self, fileobj, byteswap):
        # next 16 byte border
        fileobj.write(b'\x00' * (extstart + rawsize - fileobj.tell()))



PEP8 - two lines between classes, two lines between functions one line between methods.

kastman · 2015-02-16T20:17:10Z

That sounds like a plan; little-endian seems to be preferred from everything I've seen (I'm not even sure how to guess endianness without some magic or prior, which meta-less tags don't have). And I think that being explicit about the VR is preferable to implicit - the intended use-case is archival, not transmission over the network, so clarity has the premium over compression or compactness.

I'm not sure about the endianness of PMOD files - everything I've seen has been little, but there's no doc or guarantee. I could write to the developers if you think it's worth it?

Should we toss reading full metadata dicom datasets until someone wants to use them? Seems like we should honor it if it's present...

matthew-brett · 2015-02-16T21:28:59Z

Thinking more - I wonder if we should default to writing as the byte order of the header (header.endianness). Of course this will almost invariably by little-endian these days but still.

I guess we could write explicit VR, header endianness always, and read any endianness, with or without full metadata. I'm neutral about allowing full metadata if you have already implemented it, just because we haven't got an example of anyone using it, and it's more code maintenance - the YAGNI principle.

kastman · 2015-02-16T21:50:59Z

There's two parts to this, the way that the header is actually written and what the header says. The dicom standard says the header is always written ExplicitVR-LittleEndian, but it can list a different method (TransferSyntax) to use for the rest of the file. However, in the case where there is no header (e.g. naked tags like PMOD) there's no TransferSyntax and no way to know the correct encoding; we can only guess it.

How about this? If there is metadata and a TransferSyntax attribute is present, write using whatever the transfer syntax says. If no TransferSyntax, always use little endian. If the dataset was inferred to be Implicit, then write with ImplicitVR, otherwise write with ExplicitVR. That way you're returning as close to what you got as possible.

I agree with you concerning YAGNI, but if I were going to use it, for archiving tags, I would use the whole dataset with metadata, and I would expect the writing to follow the Syntax - not doing so would definitely be a surprise. Plus, I've created example dicoms (~360Bytes) for testing different syntaxes directly from Nibabel's tests, so that reduces maintenance cost a little.

matthew-brett · 2015-02-16T21:55:30Z

Sorry - when I was talking about the 'header' I meant the nifti header.

Do we care about what the input transfer syntax is? I mean, endian, or implicit VR? If we specified nifti-header-endian and explicit VR was the standard, could we persuade people that was the right way to save this stuff out?

kastman · 2016-03-18T20:09:49Z

Take a look at this one, @matthew-brett , and see if makes sense. Sorry again for the delay!

Nifti1DicomExtension subclasses Nifti1Extension, but defines its own __init__ and doesn't super() to Nifti1Extensions __init__ in order to allow dicom datasets to be passed directly (see the if __class__ == 'Dataset' logic at nifti1.py#L397). Because of this, I didn't need to alter the header of Nfiti1Extension to accept a parent header. This is simpler and makes fewer changes, but I wonder if I should add it to Nifti1Extension in order to make things clearer in the future?

Also, it looks like the environment is failing. Do we need to rebase now for the CI to build?

matthew-brett · 2016-03-18T23:37:31Z

I just did a merge of the master branch (makes it simpler for you to merge into your branch).

I also did a refactor we need to do anyway to clean up importing dicom / pydicom in various places.

I made a pull request into this branch (I hope): kastman#1

Please do check what I did, comments welcome.

matthew-brett · 2016-03-18T23:42:34Z

nibabel/nifti1.py

+            self._is_little_endian = parent_hdr.endianness == '<'
+        else:
+            self._is_little_endian = True
+        if content.__class__ == Dataset:


How about if isinstance(content, Dataset): ?

matthew-brett · 2016-03-18T23:46:27Z

Converting everything to the same endianness of the header sounds good to me.

matthew-brett · 2016-03-18T23:48:15Z

Also fine to override the __init__ of Nifti1Extension, as long as you are doing the same stuff in the init, and I think you are.

Use externals version of OrderedDict for parrec. Use ``next(something)`` instead of ``something.__next__``.

* tiny-fixes: (333 commits) BF: a couple of tiny fixes MAINT: add comment specifying behaviour of shape=None to _hdr_key_dict TST: update parrec volume_labels tests to check the specific key order TST: expand dualTR parrec test to check warning FIX: fix bug in OrderedDict call within parrec get_volume_labels TST: add a test using a dummy dual TR .PAR file MAINT: change sort_info to an OrderedDict to enforce a consistent ordering for a given .PAR file ENH: support multiple TR values in PARREC headers TST: add test for parrec2nii CSV output MAINT: change parrec2nii volume label output from JSON to CSV MAINT: simplify get_volume_labels by removing per-slice attributes STY: rename get_dimension_labels to get_volume_labels DOC: fix typo in parrec2nii docstring TST: add get_dimension_label tests to the 5D and 6D data sets used for testing strict_sort STY: trim dynamic_keys via list comprehension TST: test_header_dimension_labels() added STY: clarify/streamline code based on feedback FIX: bugfix to replace np.unique() with _unique_rows() for 2D inputs needed for sorting vector properties. proper 2D ndarray to list of lists for JSON export in parrec2nii TST: add sorted_labels property to FakeHeader in test_parrec.py ENH: add sorted_labels function to PARRECHeader and corresponding info to PARRECArrayProxy ... Conflicts: Changelog nibabel/tests/test_nifti1.py

Move logic for conditionally importing dicom or pydicom into own module, and use this module where we are using dicom routines.

My my pep8 is picky.

MRG: merged current master, refactor dicom import

kastman · 2016-03-19T03:20:38Z

Not sure the docstring is super-helpful - take a look? Also, fixed the value import for pydicom <1.

matthew-brett · 2016-03-19T05:36:43Z

nibabel/nifti1.py

+        """
+        Parameters
+        ----------
+        code : int|str


Detail point, but I think these should be int or str : see https://github.com/numpy/numpy/blob/master/doc/HOWTO_DOCUMENT.rst.txt#id5

Sure, I was just following the docstring convention from Nifti1Extension (nifti1.py#L262). I'm fixing that here too. Thanks for the detailed numpy docstring link; it was helpful (I know it's linked at the nibabel developer help as well, but hadn't gone back to that yet).

kastman · 2016-03-23T23:14:13Z

@matthew-brett Anything left that I can tweak or help you with on this one?

matthew-brett · 2016-03-24T02:00:29Z

Sorry to be a bit slow - looks good - thanks a lot for your persistence.

Add Nifti1DicomExtension + test

0d115dd

kastman changed the title ~~Add Nifti1DicomExtension + test~~ ENH: Add Nifti1DicomExtension + test Feb 12, 2015

kastman added 3 commits February 12, 2015 21:54

Add @dicom_test decorator to Pass w/o pydicom

6bb904d

Use BytesIO (py3k compatibility)

f591ee3

Fix BytesIO import (again), py3.3 compat

267304c

Fix writing typo, add writing tests

dbe3946

Zeropad Extension to 16 bytes, test writing

b5b6550

Also, fix _guess_implicit_VR method.

kastman added 2 commits February 14, 2015 10:13

A little cleanup

a2719d3

* Remove redundant get size method (inherited) * Remove unnecessary super()

Read full datasets (with TransferSyntax)

32fd4b1

Use write_dataset for pydicom < 0.9.9 compat

bf75e49

matthew-brett reviewed Feb 16, 2015
View reviewed changes

PEP8 Whitespace

9d0cdee

NiftiHeader determines dicom byte encoding in extension

44f7430

matthew-brett reviewed Mar 18, 2016
View reviewed changes

matthew-brett and others added 10 commits March 18, 2016 16:54

BF: a couple of tiny fixes

0f897b7

Use externals version of OrderedDict for parrec. Use ``next(something)`` instead of ``something.__next__``.

RF: move dicom / pydicom imports into own module

2065503

Move logic for conditionally importing dicom or pydicom into own module, and use this module where we are using dicom routines.

STY: remove a couple of blank lines for PEP8

14af19c

My my pep8 is picky.

Merge pull request #1 from matthew-brett/update-plus-for-296

8534bb4

MRG: merged current master, refactor dicom import

DOC Add docstring to DicomExtension __init__

65de3fe

RF Type cleanup from MB’s suggestions

674bb25

BF Import pydicom.values if first import is successful

5e37a58

BF Correct dicom import

baddbde

TST Assert TypeError for bad content type

8fb8616

matthew-brett reviewed Mar 19, 2016
View reviewed changes

kastman added 3 commits March 19, 2016 23:31

BF Remove unnecessary VR validation

d87ee32

DOC Correct docstring per numpy guidelines

e066203

TST Add empty content test case

71a3ce4

matthew-brett merged commit e1aea51 into nipy:master Mar 24, 2016

		@@ -380,6 +380,113 @@ def write_to(self, fileobj, byteswap):
		# next 16 byte border
		fileobj.write(b'\x00' * (extstart + rawsize - fileobj.tell()))

ENH: Add Nifti1DicomExtension + test #296

ENH: Add Nifti1DicomExtension + test #296

Uh oh!

Conversation

kastman commented Feb 12, 2015

Uh oh!

coveralls commented Feb 12, 2015

Uh oh!

matthew-brett commented Feb 12, 2015

Uh oh!

kastman commented Feb 13, 2015

Uh oh!

matthew-brett commented Feb 13, 2015

Uh oh!

kastman commented Feb 13, 2015

Uh oh!

coveralls commented Feb 13, 2015

Uh oh!

coveralls commented Feb 13, 2015

Uh oh!

coveralls commented Feb 14, 2015

Uh oh!

coveralls commented Feb 16, 2015

Uh oh!

coveralls commented Feb 16, 2015

Uh oh!

matthew-brett commented Feb 16, 2015

Uh oh!

matthew-brett Feb 16, 2015

Choose a reason for hiding this comment

Uh oh!

kastman Feb 16, 2015

Choose a reason for hiding this comment

Uh oh!

matthew-brett Feb 16, 2015

Choose a reason for hiding this comment

Uh oh!

kastman commented Feb 16, 2015

Uh oh!

matthew-brett commented Feb 16, 2015

Uh oh!

matthew-brett Feb 16, 2015

Choose a reason for hiding this comment

Uh oh!

kastman commented Feb 16, 2015

Uh oh!

matthew-brett commented Feb 16, 2015

Uh oh!

kastman commented Feb 16, 2015

Uh oh!

matthew-brett commented Feb 16, 2015

Uh oh!

kastman commented Mar 18, 2016

Uh oh!

matthew-brett commented Mar 18, 2016

Uh oh!

matthew-brett Mar 18, 2016

Choose a reason for hiding this comment

Uh oh!

matthew-brett commented Mar 18, 2016

Uh oh!

matthew-brett commented Mar 18, 2016

Uh oh!

kastman commented Mar 19, 2016

Uh oh!

matthew-brett Mar 19, 2016

Choose a reason for hiding this comment

Uh oh!

kastman Mar 20, 2016

Choose a reason for hiding this comment

Uh oh!

kastman commented Mar 23, 2016

Uh oh!

matthew-brett commented Mar 24, 2016

Uh oh!

Uh oh!