FIX: Set TRK byte order to little-endian before filling values #782

effigies · 2019-07-30T03:02:08Z

TRK header creation is done such that the little-endian byte order is enforced after some default values are generated.

This change sets the byte order before any non-zero values are set.

Fixes #775.

@yarikoptic @sanjayankur31 If either of you has a minute to test these changes on an s390x, it would be most appreciated.

yarikoptic · 2019-07-30T06:18:53Z

Doctests fail on Travis now

Expected:
    dtype('<i2')
Got:
    '<i2'

effigies · 2019-07-30T11:09:52Z

Ah right.

yarikoptic · 2019-07-30T15:29:53Z

This resolves the issue for me, but while testing ran into another one for a doctest of nibabel.brikhead.AFNIHeader.get_data_scaling . I will do a thorough sweep and report separately in not a fluke

(sid_s390x-dchroot)yoh@zelenka:~/nibabel$ python -m nose -s -v --with-doctest  /home/yoh/nibabel/nibabel/tests/test_scripts.py nibabel/brikhead.py
nibabel.tests.test_scripts.test_nib_ls ... ok
nibabel.tests.test_scripts.test_nib_ls(['-H', 'dim,bitpix'], ' \\[  4 128  96  24   2   1   1   1\\] 16') ... ok
nibabel.tests.test_scripts.test_nib_ls(['-c'], '', ' !1030 uniques. Use --all-counts') ... ok
nibabel.tests.test_scripts.test_nib_ls(['-c', '--all-counts'], '', ' 2:3 3:2 4:1 5:1.*') ... ok
nibabel.tests.test_scripts.test_nib_ls(['-c', '-s', '--all-counts'], '', ' \\[229725\\] \\[2, 1.2e\\+03\\] 2:3 3:2 4:1 5:1.*') ... ok
nibabel.tests.test_scripts.test_nib_ls(['-c', '-s', '-z', '--all-counts'], '', ' \\[589824\\] \\[0, 1.2e\\+03\\] 0:360099 2:3 3:2 4:1 5:1.*') ... ok
nibabel.tests.test_scripts.test_nib_ls_multiple ... SKIP: For the other tests should be able to load MINC files
nibabel.tests.test_scripts.test_help ... ok
nibabel.tests.test_scripts.test_nib_diff ... ok
nibabel.tests.test_scripts.test_nib_nifti_dx ... ok
nibabel.tests.test_scripts.test_parrec2nii ... ok
nibabel.tests.test_scripts.test_parrec2nii_with_data ... SKIP: Skipping test: test_parrec2nii_with_data: Need files in /home/yoh/nibabel/nibabel-data/nitest-balls1 for these tests
nibabel.tests.test_scripts.test_nib_trk2tck ... ok
nibabel.tests.test_scripts.test_nib_tck2trk ... ok
Doctest: nibabel.brikhead.AFNIHeader.__init__ ... ok
Doctest: nibabel.brikhead.AFNIHeader.get_affine ... ok
Doctest: nibabel.brikhead.AFNIHeader.get_data_scaling ... FAIL
Doctest: nibabel.brikhead.AFNIHeader.get_volume_labels ... ok
Doctest: nibabel.brikhead.AFNIImage ... ok
Doctest: nibabel.brikhead._unpack_var ... ok
Doctest: nibabel.brikhead.parse_AFNI_header ... ok

======================================================================
FAIL: Doctest: nibabel.brikhead.AFNIHeader.get_data_scaling
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/usr/lib/python2.7/doctest.py", line 2224, in runTest
    raise self.failureException(self.format_failure(new.getvalue()))
AssertionError: Failed doctest test for nibabel.brikhead.AFNIHeader.get_data_scaling
  File "/home/yoh/nibabel/nibabel/brikhead.py", line 414, in get_data_scaling

----------------------------------------------------------------------
File "/home/yoh/nibabel/nibabel/brikhead.py", line 422, in nibabel.brikhead.AFNIHeader.get_data_scaling
Failed example:
    header.get_data_scaling()
Expected:
    array([  3.88336300e-08])
Got:
    array([3.883363e-08])

effigies · 2019-07-30T15:42:26Z

What version of numpy is installed there?

yarikoptic · 2019-07-30T16:41:33Z

that issue is unrelated so I will continue in #784

effigies · 2019-07-30T16:59:12Z

Thanks for checking, Yarik.

@MarcCote @matthew-brett Just as a sanity check, does this make sense to you?

codecov · 2019-07-31T03:47:06Z

Codecov Report

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

@@            Coverage Diff             @@
##           master     #782      +/-   ##
==========================================
+ Coverage   89.99%   89.99%   +<.01%     
==========================================
  Files          94       94              
  Lines       12009    12015       +6     
  Branches     2133     2134       +1     
==========================================
+ Hits        10807    10813       +6     
  Misses        859      859              
  Partials      343      343

Impacted Files	Coverage Δ
nibabel/brikhead.py	`97.48% <ø> (ø)`	⬆️
nibabel/streamlines/trk.py	`94.42% <100%> (+0.11%)`	⬆️

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 b356a64...927b6ce. Read the comment docs.

matthew-brett · 2019-07-31T08:18:06Z

nibabel/streamlines/trk.py

@@ -395,8 +396,7 @@ def save(self, fileobj):
            pointing to TRK file (and ready to write from the beginning
            of the TRK header data).
        """
-        # Enforce little-endian byte order for header
-        header = self._default_structarr().newbyteorder('<')
+        header = self._default_structarr()


I know it ends up the same, but maybe cleaner keeping the endian change in the save function with

self._default_structarr().newbyteorder('<').byteswap()

Wouldn't that fail on little endian systems?

Sorry - yes. Then maybe, for clarity, add an endianness=None kwarg to _default_structarr, and pass < from save.

That works. I followed the template in WrapStruct.default_structarr.

Let me know if you have any further comments.

effigies · 2019-07-31T11:22:11Z

nibabel/streamlines/trk.py

@@ -283,10 +286,10 @@ def _default_structarr(cls):
        return st_arr

    @classmethod
-    def create_empty_header(cls):
+    def create_empty_header(cls, endianness=None):


I added this in passing, since it's the only other place _default_structarr is being called.

effigies · 2019-07-31T11:29:11Z

@yarikoptic If you get a chance, could you verify that this still works on the s390x?

yarikoptic · 2019-07-31T14:27:04Z

no good

(sid_s390x-dchroot)yoh@zelenka:~/nibabel$ git show | head
commit 3737e617d25ea1aadbd481f432ddd1a3d657a25b
Author: Christopher J. Markiewicz <[email protected]>
Date:   Wed Jul 31 07:18:24 2019 -0400

    ENH: Pass endianness as parameter to TrkFile._default_structarr

diff --git a/nibabel/streamlines/trk.py b/nibabel/streamlines/trk.py
index 3100dfe5..e43b760c 100644
--- a/nibabel/streamlines/trk.py
+++ b/nibabel/streamlines/trk.py
(sid_s390x-dchroot)yoh@zelenka:~/nibabel$ python -m nose -s -v --with-doctest  /home/yoh/nibabel/nibabel/tests/test_scripts.py:test_nib_tck2trk
nibabel.tests.test_scripts.test_nib_tck2trk ... ERROR

======================================================================
ERROR: nibabel.tests.test_scripts.test_nib_tck2trk
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/usr/lib/python2.7/dist-packages/nose/case.py", line 197, in runTest
    self.test(*self.arg)
  File "/home/yoh/nibabel/nibabel/tests/test_scripts.py", line 488, in test_nib_tck2trk
    trk = nib.streamlines.load(standard_trk)
  File "/home/yoh/nibabel/nibabel/streamlines/__init__.py", line 96, in load
    return tractogram_file.load(fileobj, lazy_load=lazy_load)
  File "/home/yoh/nibabel/nibabel/streamlines/trk.py", line 375, in load
    arr_seqs = create_arraysequences_from_generator(trk_reader, n=3)
  File "/home/yoh/nibabel/nibabel/streamlines/array_sequence.py", line 387, in create_arraysequences_from_generator
    for data in gen:
  File "/home/yoh/nibabel/nibabel/streamlines/trk.py", line 686, in _read
    buffer=f.read(nb_pts * pts_and_scalars_size))
TypeError: buffer is too small for requested array

----------------------------------------------------------------------
Ran 1 test in 0.172s

FAILED (errors=1)

effigies · 2019-07-31T17:31:47Z

@yarikoptic Woops. Forgot to actually apply the endianness. Another try?

effigies · 2019-08-01T20:03:03Z

@yarikoptic Just a bump on this.

effigies · 2019-08-01T21:41:35Z

Not sure if that's the output you meant to paste...

yarikoptic · 2019-08-01T22:09:14Z

ha ha -- indeed... "too fast" or "too tired"... removed -- thanks for letting me know. the test has passed ;-) I repeat -- the test has passed! ;)

effigies · 2019-08-01T22:11:05Z

Sounds good. Thanks for confirmation.

@matthew-brett Any remaining thoughts?

matthew-brett · 2019-08-01T23:35:33Z

Looks fine to me, thanks.

effigies · 2019-08-02T00:12:02Z

Great, thanks to both of you.

FIX: Set TRK byte order to < before filling values

71f29d3

effigies mentioned this pull request Jul 30, 2019

Test failure on s390x: test_nib_tck2trk #775

Closed

effigies added this to the 2.5.0 milestone Jul 30, 2019

DOCTEST: Display system endianness-neutral representation of <i2

2434adb

effigies force-pushed the fix/s390x_trk_bug branch from 13707d3 to 2434adb Compare July 30, 2019 11:43

yarikoptic mentioned this pull request Jul 30, 2019

doctest AFNIHeader.get_data_scaling fails if ran "alone" #784

Closed

matthew-brett reviewed Jul 31, 2019

View reviewed changes

effigies commented Jul 31, 2019

View reviewed changes

ENH: Pass endianness as parameter to TrkFile._default_structarr

927b6ce

effigies force-pushed the fix/s390x_trk_bug branch from 3737e61 to 927b6ce Compare July 31, 2019 17:31

effigies merged commit f3d115f into nipy:master Aug 2, 2019

effigies deleted the fix/s390x_trk_bug branch August 2, 2019 00:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

FIX: Set TRK byte order to little-endian before filling values #782

FIX: Set TRK byte order to little-endian before filling values #782

effigies commented Jul 30, 2019

yarikoptic commented Jul 30, 2019

effigies commented Jul 30, 2019

yarikoptic commented Jul 30, 2019 •

edited

Loading

effigies commented Jul 30, 2019

yarikoptic commented Jul 30, 2019

effigies commented Jul 30, 2019

codecov bot commented Jul 31, 2019 •

edited

Loading

matthew-brett Jul 31, 2019

effigies Jul 31, 2019

matthew-brett Jul 31, 2019

effigies Jul 31, 2019

effigies Jul 31, 2019

effigies commented Jul 31, 2019

yarikoptic commented Jul 31, 2019

effigies commented Jul 31, 2019

effigies commented Aug 1, 2019

effigies commented Aug 1, 2019

yarikoptic commented Aug 1, 2019

effigies commented Aug 1, 2019

matthew-brett commented Aug 1, 2019

effigies commented Aug 2, 2019

FIX: Set TRK byte order to little-endian before filling values #782

FIX: Set TRK byte order to little-endian before filling values #782

Conversation

effigies commented Jul 30, 2019

yarikoptic commented Jul 30, 2019

effigies commented Jul 30, 2019

yarikoptic commented Jul 30, 2019 • edited Loading

effigies commented Jul 30, 2019

yarikoptic commented Jul 30, 2019

effigies commented Jul 30, 2019

codecov bot commented Jul 31, 2019 • edited Loading

Codecov Report

matthew-brett Jul 31, 2019

Choose a reason for hiding this comment

effigies Jul 31, 2019

Choose a reason for hiding this comment

matthew-brett Jul 31, 2019

Choose a reason for hiding this comment

effigies Jul 31, 2019

Choose a reason for hiding this comment

effigies Jul 31, 2019

Choose a reason for hiding this comment

effigies commented Jul 31, 2019

yarikoptic commented Jul 31, 2019

effigies commented Jul 31, 2019

effigies commented Aug 1, 2019

effigies commented Aug 1, 2019

yarikoptic commented Aug 1, 2019

effigies commented Aug 1, 2019

matthew-brett commented Aug 1, 2019

effigies commented Aug 2, 2019

yarikoptic commented Jul 30, 2019 •

edited

Loading

codecov bot commented Jul 31, 2019 •

edited

Loading