BF: if voxel order in header is lowercase, .trk can't load #590

kesshijordan · 2018-01-10T02:54:55Z

If the voxel_order field of the header is lowercase in a track file (this is the case when I re-save streamlines using trackvis on my mac: Trackvis Version 0.6.1 (Build 2017.01.17) Public), it will not load using the new nib.streamlines.load because axcodes2ornt returns nan's when given a lower case voxel order.

… file

coveralls · 2018-01-10T03:44:04Z

Coverage increased (+0.004%) to 96.337% when pulling 419998a on kesshijordan:fix_vo_case into 6d54618 on nipy:master.

matthew-brett · 2018-01-11T16:56:00Z

Thanks a lot for this - @MarcCote - any reason we should insist on lower-case voxel orientations?

@kesshijordan - any chance of adding a test?

kesshijordan · 2018-01-11T17:32:28Z

Thanks, @matthew-brett. Sure, though do you know how to fix the travis failures, above? The spacing on the arrays disagrees on expected vs got (though the numbers match) in the DOC_DOC_TEST=1 builds.

effigies · 2018-01-11T17:41:32Z

Looks like that's an effect of numpy releasing 1.14 (related: #556, #582), and us not running DOC_DOC_TEST on prereleases. I wouldn't worry about it in this PR.

codecov-io · 2018-01-11T20:19:28Z

Codecov Report

Merging #590 into master will decrease coverage by 0.11%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #590      +/-   ##
==========================================
- Coverage   94.58%   94.46%   -0.12%     
==========================================
  Files         177      177              
  Lines       25533    24949     -584     
  Branches     2813     2661     -152     
==========================================
- Hits        24150    23569     -581     
  Misses        908      908              
+ Partials      475      472       -3

Impacted Files	Coverage Δ
nibabel/tests/test_orientations.py	`100% <100%> (ø)`	⬆️
nibabel/streamlines/tests/test_trk.py	`100% <100%> (ø)`	⬆️
nibabel/orientations.py	`97.5% <100%> (+0.02%)`	⬆️
nibabel/streamlines/trk.py	`94.29% <0%> (-1.97%)`	⬇️
nibabel/streamlines/tests/test_tractogram.py	`97.75% <0%> (-0.68%)`	⬇️
nibabel/streamlines/tests/test_array_sequence.py	`99.5% <0%> (-0.5%)`	⬇️
nibabel/streamlines/tck.py	`98.89% <0%> (-0.04%)`	⬇️
nibabel/tests/test_volumeutils.py	`98.94% <0%> (ø)`	⬆️
nibabel/streamlines/utils.py	`100% <0%> (ø)`	⬆️
nibabel/streamlines/tractogram.py	`99.66% <0%> (+0.65%)`	⬆️
... and 1 more

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 6d54618...9a74880. Read the comment docs.

kesshijordan · 2018-01-11T20:29:17Z

Thanks, @effigies

@matthew-brett I added a test comparing the trackvis to rasmm affines between two simulated trk files (voxel order las and LAS). I put it in the test_load_file_with_wrong_information() set. Is that fine?

coveralls · 2018-01-11T21:36:24Z

Coverage increased (+0.03%) to 96.361% when pulling 9a74880 on kesshijordan:fix_vo_case into 6d54618 on nipy:master.

MarcCote · 2018-01-12T15:43:58Z

@matthew-brett no, there is no reason why we couldn't support both. That said, shouldn't we fix that in nibabel.orientations.axcodes2ornt? Is there any reason why we couldn't support lower case letter for the orientation?

matthew-brett · 2018-01-12T17:04:54Z

Good point - lower case should be OK for the axis codes in orientations.py ...

…ns are supported

kesshijordan · 2018-01-16T21:10:56Z

@matthew-brett and @MarcCote : I moved the change to nibabel.orientations.axcodes2ornt by inserting axcodes = axcodes.upper() as the first line. I think the test I added before should still work, since it would break if 'las' and 'LAS' were treated differently. Is this okay?

MarcCote · 2018-01-19T20:42:09Z

nibabel/orientations.py

@@ -349,6 +349,7 @@ def axcodes2ornt(axcodes, labels=None):
           [ 0., -1.],
           [ 2.,  1.]])
    """
+    axcodes = axcodes.upper()


According to the docsring, axcodes is a tuple not a str. So, .upper() does not exist.

Thanks; I hadn't noticed that : ) It fixed my loading issue because the .trk files I'm trying to load have orientation stored as a string, but that does break on tuples. I exchanged that line for a loop that generates an upper-case tuple from either a string or a tuple (see below). How's this?

upper_str = [] for c in axcodes: upper_str.append(c.upper()) axcodes = tuple(upper_str)

How about the following?

axcodes = tuple(c.upper() for c in axcodes)

Thanks, @effigies

…n axcodes2ornt

effigies · 2018-01-23T18:14:07Z

Sorry to push more work on you, but do you think you could quickly add some test cases to axcodes2ornt?

nibabel/nibabel/tests/test_orientations.py

Lines 278 to 314 in ee4ce2a

    
           def test_axcodes2ornt(): 
        
               # Go from axcodes back to orientations 
        
               labels = (('left', 'right'), ('back', 'front'), ('down', 'up')) 
        
               assert_array_equal(axcodes2ornt(('right', 'front', 'up'), labels), 
        
                                  [[0, 1], 
        
                                   [1, 1], 
        
                                   [2, 1]] 
        
                                  ) 
        
               assert_array_equal(axcodes2ornt(('left', 'back', 'down'), labels), 
        
                                  [[0, -1], 
        
                                   [1, -1], 
        
                                   [2, -1]] 
        
                                  ) 
        
               assert_array_equal(axcodes2ornt(('down', 'back', 'left'), labels), 
        
                                  [[2, -1], 
        
                                   [1, -1], 
        
                                   [0, -1]] 
        
                                  ) 
        
               assert_array_equal(axcodes2ornt(('front', 'down', 'right'), labels), 
        
                                  [[1, 1], 
        
                                   [2, -1], 
        
                                   [0, 1]] 
        
                                  ) 
        
               # default is RAS output directions 
        
               assert_array_equal(axcodes2ornt(('R', 'A', 'S')), 
        
                                  [[0, 1], 
        
                                   [1, 1], 
        
                                   [2, 1]] 
        
                                  ) 
        
               # dropped axes produce None 
        
               assert_array_equal(axcodes2ornt(('R', None, 'S')), 
        
                                  [[0, 1], 
        
                                   [np.nan, np.nan], 
        
                                   [2, 1]] 
        
                                  )

Something like:

lia_ornt = [[0, -1], [2, -1], [1, 1]]
for lia_axcodes in ('LIA', 'lia', ('L', 'I', 'A'), ('l', 'i', 'a')):
    assert_array_equal(axcodes2ornt(lia_axcodes), lia_ornt)

effigies · 2018-01-23T18:50:49Z

nibabel/orientations.py

@@ -349,6 +349,9 @@ def axcodes2ornt(axcodes, labels=None):
           [ 0., -1.],
           [ 2.,  1.]])
    """
+
+    axcodes = tuple(c.upper() for c in axcodes)


This is breaking on labels is not None. Probably the least disruptive approach is to move this into the if labels is None branch immediately below.

My reasoning is that, if somebody's passing labels, they'll also be passing axis codes with labels selected from that set. (The alternative would be to also apply .upper() to the labels, but I don't like the smell of that.)

effigies · 2018-01-29T16:41:10Z

Hi @kesshijordan, just checking in. I think this is very nearly ready, if you have a few minutes. If you're swamped, I can submit a PR to your branch with my suggested changes.

kesshijordan · 2018-01-30T01:00:22Z

Hi, @effigies. Thanks for checking in; I've been swamped but I have a bit of time now. I'll submit changes soon.

effigies

Thanks for your work on this. I think this final fix should get the tests working.

effigies · 2018-01-30T01:41:37Z

nibabel/orientations.py

@@ -349,7 +349,9 @@ def axcodes2ornt(axcodes, labels=None):
           [ 0., -1.],
           [ 2.,  1.]])
    """
+


Just for a clean diff, could you remove this empty line?

effigies · 2018-01-30T01:54:01Z

nibabel/orientations.py

    if labels is None:
+        axcodes = tuple(c.upper() for c in axcodes)


Looks like we did not consider c is None. Maybe update this to:

axcodes = tuple(c.upper() if c is not None else None for c in axcodes)

kesshijordan · 2018-01-30T01:59:09Z

Absolutely. Thanks for all the pointers, @effigies. I have made those corrections and resubmitted.

effigies · 2018-01-30T14:15:54Z

Looks ready to me. @MarcCote @matthew-brett any further comments?

matthew-brett

At least we should document that the labels / codes are not case sensitive...

matthew-brett · 2018-01-30T18:15:57Z

nibabel/orientations.py

@@ -350,6 +350,7 @@ def axcodes2ornt(axcodes, labels=None):
           [ 2.,  1.]])
    """
    if labels is None:
+        axcodes = tuple(c.upper() if c is not None else None for c in axcodes)


Should this be run only when labels is None?

Yes. Unless the labels themselves are also to be filtered. See #590 (comment).

Doesn't that smell off though - to be case insensitive only when the labels are not specified?

I guess the ideal would be to be case insensitive only for codes labels in LRAPIS.

I see, so the issue is if someone passes an alternative default ordering of the normal codes, but not custom labels, we want the overall behavior to be unchanged. I was under the impression that the only time you wanted to specify labels would be to use some custom labeling.

Then yeah, that might make sense. What if it becomes:

if labels is None: labels = list(zip('LPI', 'RAS')) # Enable case-insensitive axis codes for standard direction labels if set(elem for pair in labels for elem in pair) == set('LRAPIS'): axcodes = tuple(...)

@matthew-brett Can you please confirm whether I've summarized your concern and proposed a reasonable solution? If so, I think we can get this fix in pretty soon.

Sorry to be a bit imprecise.

Now I think about this more, it still seems ugly to me to special case for characters LRAPIS. Why these and not other codes? For example, what if someone uses F and B for front and back? Wouldn't they expect the same behavior?

I've very sorry to keep changing, but can't we go back to expecting upper case for the codes from the caller, and, in particular, fixing this in the trackvis caller rather than here?

@matthew-brett would it make sense to put in a "header format check" that returns a more informative error? That way the user can see why the load is failing and modify the header themself?

matthew-brett · 2018-02-19T10:11:38Z

@kesshijordan - I've made an alternative PR, as a suggestion. It's got your initial changes, plus (I think) some code to give a more informative error, as you suggested above. Is that OK with you?

matthew-brett · 2018-02-19T10:12:08Z

Sorry - the PR is linked below, but for completeness - #600

kesshijordan · 2018-02-20T05:22:59Z

That sounds good, @matthew-brett. Thanks!

nibotmi · 2018-02-20T12:50:56Z

☔ The latest upstream changes (presumably #600) made this pull request unmergeable. Please resolve the merge conflicts.

matthew-brett · 2018-02-20T12:54:48Z

@kesshijordan - OK to close this one?

MarcCote · 2018-02-20T15:09:34Z

Yes, I'm good with the changes of #600.

kesshijordan · 2018-02-20T20:27:22Z

Yes, @matthew-brett. Okay to close this one.

forced voxel order to be upper case when read from the header in .trk…

419998a

… file

added test for loading .trk with lowercase voxel order in hdr

4740545

moved the .upper() to axcodes2ornt so that lowercase voxel orientatio…

c3d86a5

…ns are supported

MarcCote approved these changes Jan 19, 2018

View reviewed changes

MarcCote suggested changes Jan 19, 2018

View reviewed changes

kesshijordan added 2 commits January 23, 2018 08:45

changed upper case modification to support tuples, not just strings i…

1bb0752

…n axcodes2ornt

loop to comprehension

63cee0a

effigies reviewed Jan 23, 2018

View reviewed changes

kesshijordan added 2 commits January 29, 2018 17:04

added test for tuple/string and upper/lowercase axcodes

55713a0

fix issue: breaks when labels is not None.

540983d

effigies reviewed Jan 30, 2018

View reviewed changes

removed empty line; fixed condition c is None

9a74880

effigies approved these changes Jan 30, 2018

View reviewed changes

matthew-brett reviewed Jan 30, 2018

View reviewed changes

matthew-brett mentioned this pull request Feb 19, 2018

MRG: fix lower-case TRK codes, add error message #600

Merged

kesshijordan closed this Feb 20, 2018

		if labels is None:
		axcodes = tuple(c.upper() for c in axcodes)

BF: if voxel order in header is lowercase, .trk can't load #590

BF: if voxel order in header is lowercase, .trk can't load #590

Conversation

kesshijordan commented Jan 10, 2018

coveralls commented Jan 10, 2018

matthew-brett commented Jan 11, 2018

kesshijordan commented Jan 11, 2018

effigies commented Jan 11, 2018

codecov-io commented Jan 11, 2018 • edited Loading

Codecov Report

kesshijordan commented Jan 11, 2018

coveralls commented Jan 11, 2018 • edited Loading

MarcCote commented Jan 12, 2018

matthew-brett commented Jan 12, 2018

kesshijordan commented Jan 16, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

effigies commented Jan 23, 2018

Choose a reason for hiding this comment

effigies commented Jan 29, 2018

kesshijordan commented Jan 30, 2018

effigies left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kesshijordan commented Jan 30, 2018

effigies commented Jan 30, 2018

matthew-brett left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matthew-brett commented Feb 19, 2018

matthew-brett commented Feb 19, 2018

kesshijordan commented Feb 20, 2018

nibotmi commented Feb 20, 2018

matthew-brett commented Feb 20, 2018

MarcCote commented Feb 20, 2018

kesshijordan commented Feb 20, 2018

codecov-io commented Jan 11, 2018 •

edited

Loading

coveralls commented Jan 11, 2018 •

edited

Loading