FIX: Accommodate Python 3 in DICOM slice sorting

[reddigari]

Looks like list.sort() in Python 3 requires a key instead of cmp parameter, which has to be given as a keyword argument.

The function that uses them (nicom.dicomreaders.slices_to_series()) doesn't have a test, and I wasn't sure whether there was any appropriate test data to use in one, so I didn't write one. I'd be happy to if someone has thoughts on what data to use.

[coveralls]

Coverage increased (+0.2%) to 92.099% when pulling 5a30076 on reddigari:master into ad0b13f on nipy:master.

[effigies]

Thanks for this. Can you provide a minimal reproduction of the issue you ran into? Given that, it might be easier to write a test.

[reddigari]

I can't provide the DICOMs I was using, but any should do:

import glob
from nibabel.nicom import dicomreaders

fnames = glob.glob("*.dcm")
wrappers = [dicomreaders.wrapper_from_file(f) for f in fnames]
series = dicomreaders.slices_to_series(wrappers)

Output before commit:

Traceback (most recent call last):
  File nibabel_test.py, line 9, in <module>
    series = dicomreaders.slices_to_series(wrappers)
  File /home/brainspec/Tools/nibabel/nibabel/nicom/dicomreaders.py, line 149, in slices_to_series
    vol_list.sort(_slice_sorter)
TypeError: must use keyword argument for key function

[effigies]

Cool. There are some .dcm files in nibabel/tests/data/, if you'd like to put together a test. That file looks pretty under-covered.

[codecov-io]

Codecov Report

Merging #728 into master will increase coverage by 0.17%.
The diff coverage is 66.66%.

@@            Coverage Diff             @@
##           master     #728      +/-   ##
==========================================
+ Coverage   89.39%   89.56%   +0.17%     
==========================================
  Files          93       93              
  Lines       11476    11511      +35     
  Branches     1992     2002      +10     
==========================================
+ Hits        10259    10310      +51     
+ Misses        881      861      -20     
- Partials      336      340       +4

Impacted Files	Coverage Δ
nibabel/nicom/dicomreaders.py	59.18% <66.66%> (+16.32%)	⬆️
nibabel/cmdline/parrec2nii.py	32.73% <0%> (ø)	⬆️
nibabel/testing/__init__.py	96.18% <0%> (+1.39%)	⬆️

---

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 ad0b13f...5a30076. Read the comment docs.

[reddigari]

Apologies for the gross derivation of the test data path, all the other tests in that suite use data in nicom/tests/data, so I had to use relative paths back up to nibabel/tests/data.

Not sure why coverage didn't change; I can't find any other tests that call slices_to_series.

[effigies]

So it looks like there are actually separate DICOMs in nibabel/nicom/tests/data, and this directory is already available through the IO_DATA_PATH variable.

As to the coverage, it did update, but because AppVeyor is failing, CodeCov isn't updating the comment.

The AppVeyor failure looks unrelated, so don't worry about that.

[reddigari]

It was my understanding that this function is meant to take single-slice dicoms from one or more series, separate them into series, and then sort the slices (apparently only along the z-dimension). The two test files I used in nibabel/tests/data are from the same series, so I thought they'd be the better choice.

Additionally, one of the two dicoms in nibabel/nicom/tests/data is a multiframe dicom, so it's less clear to me how the slice sorter should handle it.

Let me know your thoughts on how/whether to proceed. Perhaps we can copy the two files I used into the nicom test data directory, use all the .dcm files (except maybe the multiframe one), and add a test to make sure it separates the series?

[effigies]

Thanks. This looks good. I don't suppose there's any chance of digging up a file that hits the _third_pass condition?

nibabel/nibabel/nicom/dicomreaders.py

Lines 151 to 154 in 28c4576

	if len(set(zs)) < len(zs): # not unique zs
	# third pass
	out_vol_lists += _third_pass(vol_list)
	continue

[reddigari]

We could duplicate one of the slices, and append it to the list of wrappers. That should register as a duplicated z-value, and call _third_pass(). We can probably fidget with its instance_number attribute to fully cover the function.

I might be able to tackle that some time this weekend, if you think it's the right move.

[effigies]

On the whole more coverage is better, but it should be a valid DICOM. I'll confess to knowing little about the format, so I don't know if what you describe is normal or something that will technically work but isn't actually valid. Another thing to consider is looking through PyDICOM's test data directory for something that triggers this code.

[effigies]

I'm going to go ahead and merge. If you get a chance to add a new test file, it would be highly appreciated.