ENH: Support for transforms mappings (e.g., head-motion correction) #59

oesteban · 2020-02-18T02:27:17Z

Implements two types of transforms mappings:

a general one for any internal transform, and
an optimized mapping for lineart transforms (head-motion correction).

Since scipy's interpn only accepts linear and nearest interpolations,
these mappings will not support for simultaneous slice-timing correction
(STC) for the time being.

The addition of 4D tensor B-Spline basis for interpolation would allow
for simultaneous STC, HMC and SDC of functional time-series.
Not sure we should keep supporting Lanczos interpolation, very much less
if we want to do all three corrections at the same time.

Closes #46

pull-assistant · 2020-02-18T02:27:19Z

Best reviewed: commit by commit

Optimal code review plan (2 warnings)

ENH: Support for transforms mappings (e.g., head-motion correction)

nitransforms/base.py 83% changes removed in enh: remove code tha...

tst: add some tests to extend coverage

...ransforms/tests/test_linear.py 42% changes removed in tst: ramping up cove...

tst: ramping up coverage

enh: remove code that does not pertain to this PR, hit more lines

tst: add one more test

Powered by Pull Assistant. Last update d35f658 ... da44997. Read the comment docs.

pep8speaks · 2020-02-18T02:27:21Z

Hello @oesteban! Thanks for updating this PR.

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2020-02-25 17:24:43 UTC

codecov-io · 2020-02-19T21:56:16Z

Codecov Report

Merging #59 into master will decrease coverage by 0.32%.
The diff coverage is 96.72%.

@@            Coverage Diff             @@
##           master      #59      +/-   ##
==========================================
- Coverage   98.84%   98.52%   -0.33%     
==========================================
  Files          11       11              
  Lines         868      951      +83     
  Branches      111      130      +19     
==========================================
+ Hits          858      937      +79     
- Misses          6        7       +1     
- Partials        4        7       +3

Flag	Coverage Δ
#unittests	`98.52% <96.72%> (-0.33%)`	⬇️

Impacted Files	Coverage Δ
nitransforms/nonlinear.py	`100% <100%> (ø)`	⬆️
nitransforms/base.py	`99.42% <100%> (+0.02%)`	⬆️
nitransforms/io/fsl.py	`94.73% <100%> (-0.07%)`	⬇️
nitransforms/linear.py	`96.05% <96%> (-1.25%)`	⬇️

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 f8b725f...da44997. Read the comment docs.

Implements two types of transforms mappings: 1. a general one for any internal transform, and 2. an optimized mapping for lineart transforms (head-motion correction). Since scipy's interpn only accepts linear and nearest interpolations, these mappings will not support for simultaneous slice-timing correction (STC) for the time being. The addition of 4D tensor B-Spline basis for interpolation would allow for simultaneous STC, HMC and SDC of functional time-series. Not sure we should keep supporting Lanczos interpolation, very much less if we want to do all three corrections at the same time. Closes nipy#46

mgxd

very minor comments, looks great

mgxd · 2020-02-21T14:11:30Z

nitransforms/linear.py

+        if matrix is not None:
+            matrix = np.array(matrix)
+            if matrix.ndim != 2:
+                raise TypeError('Affine should be 2D.')


Suggested change

raise TypeError('Affine should be 2D.')

raise ValueError('Affine should be 2D.')

Since this matrix is the core formal element and the problem is not whether the values in the matrix make sense or are from a valid finding of inputs, this seemed to me more like a type error. Let me know if this argument is convincing

At least to me, it doesn't make sense to throw a TypeError if the matrix is an array, but it does make sense to throw a ValueError if the array dimensions aren't what is expected. But your call at end, nbd

Okay, this is my last attempt to show what I think is a ValueError in contrast to a TypeError: https://github.com/poldracklab/nitransforms/pull/59/files#diff-ca10ddac142c49ab69b356fb51a6a075R69-R70

Here, all the formal checks (i.e., shape) on the validity of the matrix pass, but the values in it are outside of the possible inputs domain.

The two type errors above could go away if we modify this object to, for instance, take a (3, 4) matrix (à la AFNI), or accept a mat+vec representation (allow the argument matrix to be (3, 3) and have an extra (3, ) vector argument for the translation).

The ValueError, though, will always be there.

This is completely a conceptual and possibly arbitrary argument - if you are still unconvinced I'll assume I'm wrong (that's the most likely possibility in that case because you are an English speaker and I'm not) and switch all to ValueErrors.

mgxd · 2020-02-21T14:11:44Z

nitransforms/linear.py

+            if matrix.ndim != 2:
+                raise TypeError('Affine should be 2D.')
+            elif matrix.shape[0] != matrix.shape[1]:
+                raise TypeError('Matrix is not square.')


Suggested change

raise TypeError('Matrix is not square.')

raise ValueError('Matrix is not square.')

mgxd · 2020-02-21T14:15:25Z

nitransforms/linear.py

-
+        if cls == Affine:
+            if np.shape(matrix)[0] != 1:
+                raise TypeError(


Suggested change

raise TypeError(

raise ValueError(

Hmmm I forgot about this one. This is indeed a ValueError under my own definitions.

nitransforms/linear.py

mgxd · 2020-02-21T15:14:53Z

nitransforms/tests/test_linear.py

+])
+def test_linear_typeerrors1(matrix):
+    """Exercise errors in Affine creation."""
+    with pytest.raises(TypeError):


Suggested change

with pytest.raises(TypeError):

with pytest.raises(ValueError):

mgxd · 2020-02-21T15:15:06Z

nitransforms/tests/test_linear.py

+
+def test_linear_typeerrors2(data_path):
+    """Exercise errors in Affine creation."""
+    with pytest.raises(TypeError):


Suggested change

with pytest.raises(TypeError):

with pytest.raises(ValueError):

oesteban

@mgxd - just a yay or nay and I'll do accordingly :)

oesteban · 2020-02-25T17:25:22Z

nitransforms/linear.py

+        if matrix is not None:
+            matrix = np.array(matrix)
+            if matrix.ndim != 2:
+                raise TypeError('Affine should be 2D.')


Okay, this is my last attempt to show what I think is a ValueError in contrast to a TypeError: https://github.com/poldracklab/nitransforms/pull/59/files#diff-ca10ddac142c49ab69b356fb51a6a075R69-R70

Here, all the formal checks (i.e., shape) on the validity of the matrix pass, but the values in it are outside of the possible inputs domain.

The two type errors above could go away if we modify this object to, for instance, take a (3, 4) matrix (à la AFNI), or accept a mat+vec representation (allow the argument matrix to be (3, 3) and have an extra (3, ) vector argument for the translation).

The ValueError, though, will always be there.

This is completely a conceptual and possibly arbitrary argument - if you are still unconvinced I'll assume I'm wrong (that's the most likely possibility in that case because you are an English speaker and I'm not) and switch all to ValueErrors.

oesteban force-pushed the enh/transform-map branch 4 times, most recently from b93a28e to 4cfb0a8 Compare February 19, 2020 21:56

oesteban force-pushed the enh/transform-map branch from 9cecb8e to 0510755 Compare February 20, 2020 02:00

oesteban added 2 commits February 20, 2020 14:19

tst: add some tests to extend coverage

2bda625

oesteban force-pushed the enh/transform-map branch from 0510755 to a8d9039 Compare February 20, 2020 23:12

tst: ramping up coverage

6bd56c5

oesteban force-pushed the enh/transform-map branch from a8d9039 to 6bd56c5 Compare February 20, 2020 23:13

oesteban marked this pull request as ready for review February 20, 2020 23:14

oesteban requested a review from mgxd February 20, 2020 23:14

enh: remove code that does not pertain to this PR, hit more lines

39ce629

mgxd approved these changes Feb 21, 2020

View reviewed changes

tst: add one more test

da44997

oesteban commented Feb 25, 2020

View reviewed changes

oesteban merged commit 955cd38 into nipy:master Feb 25, 2020

oesteban deleted the enh/transform-map branch February 25, 2020 22:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ENH: Support for transforms mappings (e.g., head-motion correction) #59

ENH: Support for transforms mappings (e.g., head-motion correction) #59

oesteban commented Feb 18, 2020

pull-assistant bot commented Feb 18, 2020 •

edited

Loading

pep8speaks commented Feb 18, 2020 •

edited

Loading

codecov-io commented Feb 19, 2020 •

edited

Loading

mgxd left a comment

mgxd Feb 21, 2020

oesteban Feb 21, 2020

mgxd Feb 21, 2020

oesteban Feb 25, 2020

mgxd Feb 21, 2020

mgxd Feb 21, 2020

oesteban Feb 25, 2020

mgxd Feb 21, 2020

mgxd Feb 21, 2020

oesteban left a comment

oesteban Feb 25, 2020

	raise TypeError('Affine should be 2D.')
	raise ValueError('Affine should be 2D.')

	raise TypeError('Matrix is not square.')
	raise ValueError('Matrix is not square.')

	with pytest.raises(TypeError):
	with pytest.raises(ValueError):

ENH: Support for transforms mappings (e.g., head-motion correction) #59

ENH: Support for transforms mappings (e.g., head-motion correction) #59

Conversation

oesteban commented Feb 18, 2020

pull-assistant bot commented Feb 18, 2020 • edited Loading

pep8speaks commented Feb 18, 2020 • edited Loading

Comment last updated at 2020-02-25 17:24:43 UTC

codecov-io commented Feb 19, 2020 • edited Loading

Codecov Report

mgxd 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oesteban left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pull-assistant bot commented Feb 18, 2020 •

edited

Loading

pep8speaks commented Feb 18, 2020 •

edited

Loading

codecov-io commented Feb 19, 2020 •

edited

Loading