Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 8 commits into from
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
419998a
forced voxel order to be upper case when read from the header in .trk…
kesshijordan Jan 10, 2018
4740545
added test for loading .trk with lowercase voxel order in hdr
kesshijordan Jan 11, 2018
c3d86a5
moved the .upper() to axcodes2ornt so that lowercase voxel orientatio…
kesshijordan Jan 16, 2018
1bb0752
changed upper case modification to support tuples, not just strings i…
kesshijordan Jan 23, 2018
63cee0a
loop to comprehension
kesshijordan Jan 23, 2018
55713a0
added test for tuple/string and upper/lowercase axcodes
kesshijordan Jan 30, 2018
540983d
fix issue: breaks when labels is not None.
kesshijordan Jan 30, 2018
9a74880
removed empty line; fixed condition c is None
kesshijordan Jan 30, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions nibabel/orientations.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be run only when labels is None?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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(...)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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?

labels = list(zip('LPI', 'RAS'))
n_axes = len(axcodes)
ornt = np.ones((n_axes, 2), dtype=np.int8) * np.nan
Expand Down
13 changes: 12 additions & 1 deletion nibabel/streamlines/tests/test_trk.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@
from ..tractogram_file import HeaderError, HeaderWarning

from .. import trk as trk_module
from ..trk import TrkFile, encode_value_in_name, decode_value_from_name
from ..trk import TrkFile, encode_value_in_name, decode_value_from_name, get_affine_trackvis_to_rasmm
from ..header import Field

DATA = {}
Expand Down Expand Up @@ -110,6 +110,17 @@ def trk_with_bytes(self, trk_key='simple_trk_fname', endian='<'):
return trk_struct, trk_bytes

def test_load_file_with_wrong_information(self):
# Simulate a TRK file where `voxel_order` is lowercase.
trk_struct1, trk_bytes1 = self.trk_with_bytes()
trk_struct1[Field.VOXEL_ORDER] = b'LAS'
trk1 = TrkFile.load(BytesIO(trk_bytes1))
trk_struct2, trk_bytes2 = self.trk_with_bytes()
trk_struct2[Field.VOXEL_ORDER] = b'las'
trk2 = TrkFile.load(BytesIO(trk_bytes2))
trk1_aff2rasmm = get_affine_trackvis_to_rasmm(trk1.header)
trk2_aff2rasmm = get_affine_trackvis_to_rasmm(trk2.header)
assert_array_equal(trk1_aff2rasmm,trk2_aff2rasmm)

# Simulate a TRK file where `count` was not provided.
trk_struct, trk_bytes = self.trk_with_bytes()
trk_struct[Field.NB_STREAMLINES] = 0
Expand Down
5 changes: 5 additions & 0 deletions nibabel/tests/test_orientations.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -313,6 +313,11 @@ def test_axcodes2ornt():
[2, 1]]
)

# test that format of axcode works for tuples and upper/lower case
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)


def test_aff2axcodes():
assert_equal(aff2axcodes(np.eye(4)), tuple('RAS'))
Expand Down