BF: deterministic order of slice_time deduction, warning if multiple match

[yarikoptic]

Initially hit while trying to make dcmstack python3 compatible: moloney/dcmstack#61
What I found that dcmstack test fails due to slice_order not matching original one (4) on some runs under Python 3. But the issue is generic -- reliance on order of keys in dict, and ignoring possible multiple "matches". It was triggered by dcmstack since its test image just has 2 slices so multiple slice orders would match.

I've looked into the logic in nibabel, and my proposed solution is just a solution:

• I think ideally Recoder class which stores those mappings starts to use OrderedDict to maintain order in which choices are specified, and then code should explore in that order. I was scared to touch Recorder class since it relies on self.__dict__ assignments
• so I just sorted choices for the slice orders based on their expanded names (since probably original order is gone by then) and in reverse so sequentials go first
• I issue a warning now that multiple choices match. I am not sure how good that is, i.e. may be the warning is not really "due" in some cases? but imho it is also bad to just choose "some" without a clear idea which one is the "correct one"

I would be interested to hear what others think about this issue, and how likely it affected anyone's real data

[coveralls]

Coverage increased (+0.005%) to 91.794% when pulling d756751 on yarikoptic:bf-sliceorder into 2a127cc on nipy:master.

[yarikoptic]

hm, appveyor failures on python 3.5 seems to not be related

[matthew-brett]

Would you mind adding a test that triggers the failure?

[codecov-io]

Codecov Report

Merging #647 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #647      +/-   ##
==========================================
+ Coverage   88.81%   88.82%   +<.01%     
==========================================
  Files          92       92              
  Lines       11278    11285       +7     
  Branches     1848     1850       +2     
==========================================
+ Hits        10017    10024       +7     
  Misses        926      926              
  Partials      335      335

Impacted Files	Coverage Δ
nibabel/volumeutils.py	92.78% <100%> (+0.02%)	⬆️
nibabel/nifti1.py	91.22% <100%> (+0.06%)	⬆️

---

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 2a127cc...d756751. Read the comment docs.

[yarikoptic]

added some basic test for guaranteeing that it is always the same, see 244c8cf (although I didn't check if it would be triggered without the fix, I assume it might/would though ;-))

[yarikoptic]

@matthew-brett @effigies - but what do you think in general, either my solution would suffice or it should be more thorough, e.g. relying on the original order of those labels in the structure? we better fix it once instead of making output inconsistent across versions if we keep fixing it up incrementally ;-)

[effigies]

My inclination would be to set a canonical ordering, rather than depend on sorting by name, and insertion order is the most intuitive way to impose this order. This preference is due to the general move in Python to ordered dictionaries, as well as lexicographical order being an artificial (and potentially annoying) constraint.

Not a strongly-held opinion though. Happy to be argued out.

[effigies]

Want to check that the warning is generated? e.g.

with clear_and_catch_warnings() as w:
    hdr2.set_slice_times([0.1, 0])
    assert len(w) == 1

[yarikoptic]

Thank you @effigies ! Do you see an easy generic way to make those Recorders to use ordered dict without duplicating storage etc?

[effigies]

I see a couple options:

1. Change the default map_maker to OrderedDict:

nibabel/nibabel/volumeutils.py

Line 81 in 2a127cc

def __init__(self, codes, fields=('code',), map_maker=dict):

2. Change the specific map_maker for slice_order_codes:

nibabel/nibabel/nifti1.py

Lines 143 to 150 in 2a127cc

	slice_order_codes = Recoder(( # code, label
	(0, 'unknown'),
	(1, 'sequential increasing', 'seq inc'),
	(2, 'sequential decreasing', 'seq dec'),
	(3, 'alternating increasing', 'alt inc'),
	(4, 'alternating decreasing', 'alt dec'),
	(5, 'alternating increasing 2', 'alt inc 2'),
	(6, 'alternating decreasing 2', 'alt dec 2')), fields=('code', 'label'))

Becomes:

slice_order_codes = Recoder((  # code, label
    (0, 'unknown'),
    (1, 'sequential increasing', 'seq inc'),
    (2, 'sequential decreasing', 'seq dec'),
    (3, 'alternating increasing', 'alt inc'),
    (4, 'alternating decreasing', 'alt dec'),
    (5, 'alternating increasing 2', 'alt inc 2'),
    (6, 'alternating decreasing 2', 'alt dec 2')),
    fields=('code', 'label'), map_maker=OrderedDict)

Not sure if this resolves your duplicated storage concern, as I'm not quite sure what that concern is. These seem like pretty small dictionaries, anyway.

[yarikoptic]

Thank you @effigies ! went 1. route, had to add OrderedSet construct, seems to work and IMHO should be "better" ;-) Hopefully noone relied on doctesting output to be a set ;-)

[effigies]

Pre-release tests are seg-faulting, but otherwise this LGTM.

[effigies]

May be getting bitten by OpenMathLib/OpenBLAS#1641. May be worth reporting upstream, probably numpy, but I haven't dug any further yet.

[effigies]

It seems that in numpy/numpy#11551 there were OpenBLAS-based build issues, but it was decided that this wasn't something that could be fixed in numpy, is that right @matthew-brett?

I'm inclined to go ahead and merge, and accept failing --pre tests for now.

[effigies]

Okay, looks like the broken wheels were rebuilt. I'm 👍 for merge.

@matthew-brett, any further comments?

[effigies]

Thanks @yarikoptic.