Realign4d TR from header #410

TheChymera · 2016-09-09T03:30:35Z

with "fail-safe" caveats. addressing #409

TheChymera · 2016-09-09T03:48:17Z

@matthew-brett ok, so the logic has an issue: If the TR values are non-consistent, tr_from_header will return None, after the initial tr is None check.

Checking for None twice seems really clunky. I'd prefer to just raise an exception in tr_from_header - or do you see any purpose in it returning None?

matthew-brett · 2016-09-09T07:25:05Z

It will be more obvious to you, doing the implementation, what the cleanest way is - please feel free to chose what you prefer.

TheChymera · 2016-09-12T22:44:45Z

@matthew-brett It seems the checks are indeed failing because of something with the functions I've been working on. Can you help me figure out why? I don't understand what this not raising a ValueError is all about:

======================================================================
FAIL: nipy.algorithms.registration.tests.test_fmri_realign4d.test_realign4d_params
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/travis/build/nipy/nipy/venv/lib/python3.4/site-packages/nose/case.py", line 198, in runTest
    self.test(*self.arg)
  File "/home/travis/build/nipy/nipy/venv/lib/python3.4/site-packages/nipy/algorithms/registration/tests/test_fmri_realign4d.py", line 170, in test_realign4d_params
    assert_raises(ValueError, Realign4d, im, None, [0, 1, 2], None)
  File "/home/travis/build/nipy/nipy/venv/lib/python3.4/site-packages/numpy/testing/utils.py", line 1145, in assert_raises
    return nose.tools.assert_raises(*args,**kwargs)
AssertionError: ValueError not raised by Realign4d

======================================================================
FAIL: nipy.algorithms.registration.tests.test_fmri_realign4d.test_spacetimerealign_params
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/travis/build/nipy/nipy/venv/lib/python3.4/site-packages/nose/case.py", line 198, in runTest
    self.test(*self.arg)
  File "/home/travis/build/nipy/nipy/venv/lib/python3.4/site-packages/nipy/algorithms/registration/tests/test_fmri_realign4d.py", line 197, in test_spacetimerealign_params
    assert_raises(ValueError, SpaceTimeRealign, runs, None, [0, 1, 2], 2)
  File "/home/travis/build/nipy/nipy/venv/lib/python3.4/site-packages/numpy/testing/utils.py", line 1145, in assert_raises
    return nose.tools.assert_raises(*args,**kwargs)
AssertionError: ValueError not raised by SpaceTimeRealign

matthew-brett · 2016-09-13T00:09:41Z

What happens when you try running the same checks in IPython, like:

>>> SpaceTimeRealign(runs, None, [0, 1, 2], 2)

Do they generate a ValueError? Should they?

TheChymera · 2016-09-13T01:05:51Z

It generates a NameError, as I believe it should, since runs is not defined.

matthew-brett · 2016-09-13T01:26:22Z

Sure, but what if you replicate the environment in the test. I guess runs is defined there?

TheChymera · 2016-09-13T01:29:07Z

Hmmm, that gives me the output I would expect, but I am surprised that it's the last line of the exception that gets highlighted:

/home/chymera/src/nipy/nipy/algorithms/registration/groupwise_registration.py in _init(self, images, tr, slice_times, slice_info, affine_class)
    758                 raise ValueError('A TR of 1 was found in the header.\
    759                     This value often stands in for an unknown TR. Please\
--> 760                     specify TR explicitly.')
    761         elif tr is "header-allow-1.0":
    762             tr = tr_from_header(images)

ValueError: A TR of 1 was found in the header.                    This value often stands in for an unknown TR. Please                    specify TR explicitly.

Also, I don't like how that error message prints, so I just updated the PR, now it gives me this, which I guess is just as wrong/right, but looks a deal better:

In [17]: SpaceTimeRealign(img, None, [0, 1, 2], 2)
---------------------------------------------------------------------------
ValueError                                Traceback (most recent call last)
<ipython-input-17-c283a7595358> in <module>()
----> 1 SpaceTimeRealign(img, None, [0, 1, 2], 2)

/home/chymera/src/nipy/nipy/algorithms/registration/groupwise_registration.py in __init__(self, images, tr, slice_times, slice_info, affine_class)
    967         if hasattr(slice_times, '__call__'):
    968             slice_times = slice_times(n_slices, tr)
--> 969         self._init(images, tr, slice_times, slice_info, affine_class)
    970 
    971 

/home/chymera/src/nipy/nipy/algorithms/registration/groupwise_registration.py in _init(self, images, tr, slice_times, slice_info, affine_class)
    755             tr = tr_from_header(images)
    756             if tr == 1:
--> 757                 raise ValueError('A TR of 1 was found in the header. '\
    758                     'This value often stands in for an unknown TR. '\
    759                     'Please specify TR explicitly.')

ValueError: A TR of 1 was found in the header. This value often stands in for an unknown TR. Please specify TR explicitly.

matthew-brett · 2016-09-13T01:33:43Z

nipy/algorithms/registration/groupwise_registration.py

-            raise ValueError('Repetition time cannot be None')
+            tr = tr_from_header(images)
+            if tr == 1:
+                raise ValueError('A TR of 1 was found in the header. '\


You don't need the line continuation here, Python will combine the strings without them.

matthew-brett · 2016-09-13T01:35:24Z

Sorry - I see - you are seeing the errors for the tests that were expecting None to generate an error. You need to replace those tests with tests of the new TR-finding machinery.

matthew-brett · 2016-09-13T01:45:19Z

nipy/algorithms/registration/groupwise_registration.py

+            if tr != images_tr:
+                raise ValueError('TR inconsistent between images.')
+        else:
+            return images_tr


The else is a bit confusing here, you usually use this to run something when there was no break in the for loop. Of course there isn't here, but an experienced programmer will look for one. How about something like:

def tr_from_header(images): """ A good docstring """ if not isinstance(images, list): images = [images] images_tr = None for image in images: tr = image.header.get_zooms()[3] if images_tr is None: images_tr = tr if tr != images_tr: raise ValueError('TR inconsistent between images.') return images_tr

I went for no_init_value instead of the more elegant

if images_tr is None: images_tr = tr

BEcause I was worried that the logic would be unable to pick up different-TR images if the first n of the list had image.header.get_zooms()[3] == None but it seems .get_zooms() can never produce None. Can you confirm this?

Yes, that's right - it only produces a vector of numbers, one per dimension of the image.

TheChymera · 2016-09-13T01:47:37Z

Currently we only get a ValueError if the header TR is 0 or 1 - which is not the case for functional.nii.gz. can I remove the offending lines?

matthew-brett · 2016-09-13T02:35:41Z

Why not write a few tests for the states that do raise errors, such as bad TRs or TR not equal between images? You could make some example runs e.g.

import nibabel as nib
img = nib.Nifti1Image(np.zeros((2, 3, 4, 5), np.eye(4))
img.header['pixdim'][4] = 0

etc.

coveralls · 2016-09-13T15:08:14Z

Coverage decreased (-0.0002%) to 87.138% when pulling 7073b8d on TheChymera:master into 91d91f5 on nipy:master.

TheChymera · 2016-09-13T17:55:50Z

I can't really make sense of the coveralls output - how exactly did I reduce the coverage?

@matthew-brett Also, can you help me out with travis? I get this mesage:

AttributeError: 'Nifti1Image' object has no attribute 'header'

But afaict Nifti1Image objects do have a header.

matthew-brett · 2016-09-13T18:03:22Z

nipy/algorithms/registration/groupwise_registration.py

+            if tr == 1:
+                raise ValueError('A TR of 1 was found in the header. '
+                    'This value often stands in for an unknown TR. '
+                    'Please specify TR explicitly.')


Maybe add: or consider "header-allow-1.0".

matthew-brett · 2016-09-13T18:12:40Z

Sorry, the coveralls output is hard to read. I now prefer codecov, I should set that up. But I'm sure the problem is that you aren't currently testing the header-allow-1.0 branch. Try testing that option?

TheChymera · 2016-09-13T23:56:10Z

@matthew-brett ok, it seems there is yet another problem. TR is needed in SpaceTimeRealign before it is initialized.

What would you say is better - moving the slice_times calculation to Realign4d, or the entire TR logic to SpaceTimeRealign?

matthew-brett · 2016-09-14T00:23:57Z

I'd say move that logic into the _init method, and modify the docstring for Realign4d to reflect that.

codecov-io · 2016-09-14T04:30:23Z

Current coverage is 83.54% (diff: 100%)

Merging #410 into master will increase coverage by 0.01%

@@             master       #410   diff @@
==========================================
  Files           291        291          
  Lines         27757      27790    +33   
  Methods           0          0          
  Messages          0          0          
  Branches       3252       3262    +10   
==========================================
+ Hits          23185      23218    +33   
  Misses         3570       3570          
  Partials       1002       1002

Powered by Codecov. Last update 91d91f5...8ddb756

TheChymera · 2016-09-14T04:40:15Z

I added the logic to SpaceTimeRealign instead, as there was no way to separate the tr from the rest of the higher-level logic. Also, modifying the input parsing for Realign4d might have broken other classes based on it. I think it makes sense to keep Realign4d as basic as possible and do the fancy stuff in other classes.

I still don't understand what's up with codecov. The lines it highlights are tested by line 202 and 204, respectively.

matthew-brett · 2016-09-14T07:51:02Z

Not sure what's going on with the coverage. Have you tested that 'header-allow-1.0' works on an image with TR=0? EDIT - sorry - on an image with TR=1.

coveralls · 2016-09-14T13:48:58Z

Coverage increased (+0.01%) to 87.151% when pulling 5d1ff3a on TheChymera:master into 91d91f5 on nipy:master.

coveralls · 2016-09-14T16:17:30Z

Coverage increased (+0.01%) to 87.152% when pulling ea49b6b on TheChymera:master into 91d91f5 on nipy:master.

TheChymera · 2016-09-14T16:39:53Z

@matthew-brett anything else you'd like to recommend before I squash this?

matthew-brett · 2016-09-14T16:53:24Z

nipy/algorithms/registration/groupwise_registration.py

 from nibabel.affines import apply_affine

+from ...externals.six import string_types


This import should go at the top, because it's a sort-of special import, like a system import.

matthew-brett · 2016-09-14T16:57:15Z

nipy/algorithms/registration/tests/test_fmri_realign4d.py

@@ -3,14 +3,16 @@
 # vi: set ft=python sts=4 ts=4 sw=4 et:

 import warnings
+import nibabel as nib
+import numpy as np


Import numpy before nibabel (system imports first, then common imports, then package-specific imports).

coveralls · 2016-09-14T18:20:36Z

Coverage increased (+0.01%) to 87.153% when pulling f0fa8d5 on TheChymera:master into 91d91f5 on nipy:master.

coveralls · 2016-09-14T18:56:09Z

Coverage increased (+0.01%) to 87.153% when pulling 347fcad on TheChymera:master into 91d91f5 on nipy:master.

matthew-brett

Great - nearly there.

coveralls · 2016-09-14T20:14:12Z

Coverage increased (+0.02%) to 87.154% when pulling f41edbd on TheChymera:master into 91d91f5 on nipy:master.

and extended TR logic, and assed tests

coveralls · 2016-09-14T21:16:54Z

Coverage increased (+0.02%) to 87.154% when pulling 8ddb756 on TheChymera:master into 91d91f5 on nipy:master.

matthew-brett · 2016-09-14T22:38:39Z

Great - thanks a lot for your patience.

TheChymera changed the title ~~TR from header~~ Realign4d TR from header Sep 9, 2016

matthew-brett reviewed Sep 13, 2016
View reviewed changes

matthew-brett reviewed Sep 14, 2016
View reviewed changes

TheChymera force-pushed the master branch from f0fa8d5 to 347fcad Compare September 14, 2016 18:35

matthew-brett reviewed Sep 14, 2016

View reviewed changes

added TR reading functionfor SpaceTimeRealign

8ddb756

and extended TR logic, and assed tests

TheChymera force-pushed the master branch from f41edbd to 8ddb756 Compare September 14, 2016 20:57

matthew-brett merged commit 14907d2 into nipy:master Sep 14, 2016

TheChymera mentioned this pull request Sep 14, 2016

realign test code style #411

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Realign4d TR from header #410

Realign4d TR from header #410

TheChymera commented Sep 9, 2016 •

edited

Loading

TheChymera commented Sep 9, 2016

matthew-brett commented Sep 9, 2016

TheChymera commented Sep 12, 2016 •

edited

Loading

matthew-brett commented Sep 13, 2016

TheChymera commented Sep 13, 2016 •

edited

Loading

matthew-brett commented Sep 13, 2016

TheChymera commented Sep 13, 2016

matthew-brett Sep 13, 2016

matthew-brett commented Sep 13, 2016

matthew-brett Sep 13, 2016

TheChymera Sep 13, 2016

matthew-brett Sep 13, 2016

TheChymera commented Sep 13, 2016 •

edited

Loading

matthew-brett commented Sep 13, 2016

coveralls commented Sep 13, 2016

TheChymera commented Sep 13, 2016 •

edited

Loading

matthew-brett Sep 13, 2016

matthew-brett commented Sep 13, 2016

TheChymera commented Sep 13, 2016

matthew-brett commented Sep 14, 2016

codecov-io commented Sep 14, 2016 •

edited

Loading

TheChymera commented Sep 14, 2016

matthew-brett commented Sep 14, 2016 •

edited

Loading

coveralls commented Sep 14, 2016

coveralls commented Sep 14, 2016

TheChymera commented Sep 14, 2016

matthew-brett Sep 14, 2016

matthew-brett Sep 14, 2016

coveralls commented Sep 14, 2016

coveralls commented Sep 14, 2016

matthew-brett left a comment

coveralls commented Sep 14, 2016

coveralls commented Sep 14, 2016

matthew-brett commented Sep 14, 2016

		from nibabel.affines import apply_affine

		from ...externals.six import string_types

Realign4d TR from header #410

Realign4d TR from header #410

Conversation

TheChymera commented Sep 9, 2016 • edited Loading

TheChymera commented Sep 9, 2016

matthew-brett commented Sep 9, 2016

TheChymera commented Sep 12, 2016 • edited Loading

matthew-brett commented Sep 13, 2016

TheChymera commented Sep 13, 2016 • edited Loading

matthew-brett commented Sep 13, 2016

TheChymera commented Sep 13, 2016

matthew-brett Sep 13, 2016

Choose a reason for hiding this comment

matthew-brett commented Sep 13, 2016

matthew-brett Sep 13, 2016

Choose a reason for hiding this comment

TheChymera Sep 13, 2016

Choose a reason for hiding this comment

matthew-brett Sep 13, 2016

Choose a reason for hiding this comment

TheChymera commented Sep 13, 2016 • edited Loading

matthew-brett commented Sep 13, 2016

coveralls commented Sep 13, 2016

TheChymera commented Sep 13, 2016 • edited Loading

matthew-brett Sep 13, 2016

Choose a reason for hiding this comment

matthew-brett commented Sep 13, 2016

TheChymera commented Sep 13, 2016

matthew-brett commented Sep 14, 2016

codecov-io commented Sep 14, 2016 • edited Loading

Current coverage is 83.54% (diff: 100%)

TheChymera commented Sep 14, 2016

matthew-brett commented Sep 14, 2016 • edited Loading

coveralls commented Sep 14, 2016

coveralls commented Sep 14, 2016

TheChymera commented Sep 14, 2016

matthew-brett Sep 14, 2016

Choose a reason for hiding this comment

matthew-brett Sep 14, 2016

Choose a reason for hiding this comment

coveralls commented Sep 14, 2016

coveralls commented Sep 14, 2016

matthew-brett left a comment

Choose a reason for hiding this comment

coveralls commented Sep 14, 2016

coveralls commented Sep 14, 2016

matthew-brett commented Sep 14, 2016

TheChymera commented Sep 9, 2016 •

edited

Loading

TheChymera commented Sep 12, 2016 •

edited

Loading

TheChymera commented Sep 13, 2016 •

edited

Loading

TheChymera commented Sep 13, 2016 •

edited

Loading

TheChymera commented Sep 13, 2016 •

edited

Loading

codecov-io commented Sep 14, 2016 •

edited

Loading

matthew-brett commented Sep 14, 2016 •

edited

Loading