ENH: API change in ``TransformChain`` - new composition convention

[oesteban]

This PR makes the TransformChain class more consistent with the overall coordinate system, assuming that transforms are chained with the points criteria.

In other words, in the typical setup where we have estimated one initializing affine and then perhaps two levels of nonlinear deformations, when calculating the coordinates of a given index in the reference image, the last nonlinear should be applied first, then the second, and finally the affine to pull information from the moving image.

In other words, the chaining (composition) operation works exactly as a single transformation procedure.

Resolves #81.

[codecov[bot]]

Codecov Report

Merging #165 (e80bd3c) into master (e5a6b41) will increase coverage by 0.00%. The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master     #165   +/-   ##
=======================================
  Coverage   98.54%   98.55%           
=======================================
  Files          13       13           
  Lines        1169     1177    +8     
  Branches      184      184           
=======================================
+ Hits         1152     1160    +8     
  Misses         10       10           
  Partials        7        7           

Flag	Coverage Δ
unittests	98.55% <100.00%> (+<0.01%)	:arrow_up:

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
nitransforms/linear.py	96.94% <100.00%> (+0.14%)	:arrow_up:
nitransforms/manip.py	100.00% <100.00%> (ø)

---

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 e5a6b41...e80bd3c. Read the comment docs.

[effigies]

If I'm getting this right, we had previously adopted the ITK last-in-first-out ordering of transform chains (I'm guessing this was because our exemplar of transform chains occurring within a file.) and now we would like it to be more intuitively left-to-right, moving-to-fixed?

If so, is this more of an API change than a fix?

I would also use a different test for chaining. You currently are just showing that two inverse matrices cancel out, when something that will fail if we get the order wrong would be something like:

>>> chain = TransformChain(transforms=[
...     Affine.from_matvec(vec=(1, 2, 3)),
...     Affine.from_matvec(mat=[[0, 1, 0], [0, 0, 1], [1, 0, 0]]),
... ])
>>> chain.asaffine()
array([[0., 1., 0., 2.],
       [0., 0., 1., 3.],
       [1., 0., 0., 1.],
       [0., 0., 0., 1.]])

I think that should be right, since rotation after translation will rotate the translation parameters... Speaking of which, since you have two implementations of the ordering, I think we need to verify consistency:

def test_consistency():
    affine_chain = ...
    coords = ...
    chain_mapped = affine_chain.map(coords)
    affine_mapped = Affine(affine_chain.asaffine()).map(coords)
    assert np.allclose(chain_mapped, affine_mapped)

[oesteban]

If I'm getting this right, we had previously adopted the ITK last-in-first-out ordering of transform chains (I'm guessing this was because our exemplar of transform chains occurring within a file.) and now we would like it to be more intuitively left-to-right, moving-to-fixed?

Yes, I think this is a good summary -- although the convention proposed in the PR I believe should be called right-to-left, fixed-to-moving, as in the actual transformation equations:

$$ \mathbf{x}' = T\text{2}(T\text{1}(T_\text{0}(\mathbf{x}))) $$

where $\mathbf{x}$ is some $(x, y, z)$ coordinate in the fixed image's domain $\mathcal{F}$ and $\mathbf{x}'$ denotes the corresponding coordinate in the moving domain $\mathcal{M}$.

EDIT: forgot to mention that the subscript $i$ matches python indexing, and the transforms property of TransformChain then holds $[T\text{0}, T\text{1}, T\text{2}]$ in that order. Conversely, the ITK convention would be to store $[T\text{2}, T\text{1}, T\text{0}]$

is this more of an API change than a fix?

Yes.

I would also use a different test for chaining. You currently are just showing that two inverse matrices cancel out, when something that will fail if we get the order wrong would be something like:

I'll add your test :)

That said, there is a test that failed when I just updated the API - see https://github.com/nipy/nitransforms/pull/165/commits/7f1657c9841127a2b3a8eae313670f231942ea16

The test failed because I hadn't updated it to the new ordering.

Speaking of which, since you have two implementations of the ordering, I think we need to verify consistency:

If there are two different implementations of the ordering, that's my bad -- that shouldn't be. However, now I realize that the asaffine() feature was a bad idea and should probably disappear.

Instead, there should be just a compose(), consolidate() or collapse() (#89) that is smart enough to identify if there are nonlinear components and build an affine if there aren't.

[oesteban]

Since asaffine() seems to be the main stopper here, I'll go ahead and merge and we will go back into that particular issue with #89, which is coming up soon.

[effigies]

If I'm getting this right, we had previously adopted the ITK last-in-first-out ordering of transform chains (I'm guessing this was because our exemplar of transform chains occurring within a file.) and now we would like it to be more intuitively left-to-right, moving-to-fixed?

Yes, I think this is a good summary -- although the convention proposed in the PR I believe should be called right-to-left, fixed-to-moving, as in the actual transformation equations:

x′=T2(T1(T0(x)))

where x is some (x,y,z) coordinate in the fixed image's domain F and x′ denotes the corresponding coordinate in the moving domain M.

EDIT: forgot to mention that the subscript i matches python indexing, and the transforms property of TransformChain then holds [T0,T1,T2] in that order. Conversely, the ITK convention would be to store [T2,T1,T0]

With ITK, it does a stack, e.g., [norm, coreg]. In your formulation above, you would write x' = coreg(norm(x)) to get the location in the moving image corresponding to the target location in the fixed image. This corresponds to ITK holding [T0, T1], unless there's something different about how they map values inside the H5 files?

[oesteban]

With ITK, it does a stack, e.g., [norm, coreg]. In your formulation above, you would write x' = coreg(norm(x)) to get the location in the moving image corresponding to the target location in the fixed image. This corresponds to ITK holding [T0, T1], unless there's something different about how they map values inside the H5 files?

Just opened the issue above to make sure this does not fall through the cracks.