Fixed parameter preservation after identity

[hjmjohnson]

When setting a transform to represent an Identity mapping, it is crucial that the stationary component of the transform is preserved.

tfm.SetCenter( [1,2,3] );
tfm.SetIdentity();
out=tfm.GetCenter();
assert( out == [1,2,3] );

PR Checklist

• [X] No API changes were made (or the changes have been approved)
• [X] No major design changes were made (or the changes have been approved)
• [X] Added test (or behavior not changed) files (if any)

[N-Dekker]

Just for my understanding: how is that with transform types which (may not be not derived from MatrixOffsetTransformBase)? Do (or should) they always preserve the fixed parameters, whenever transform.SetIdentity() is called?

[hjmjohnson]

@N-Dekker FixedParameters should never change after being chosen, they represent the static component of the structure of the transform. Similar to template parameters for a function in C++. the "Parameters" represent the current state of the Transform (i.e. Not the structure of the transform).

The SetIdentity() function should change the state of the transform, not the structure of the transform.

Consider a displacement field transform. The "Parameters" are the vector values of the underlying image. The "FixedParameters" are the Size/Origin/Spacing/Direction components that define the underlying vector image. Setting the displacement field to an identity involves setting the vector values to all zeros, and should not modify the Size/Origin/Spacing/Direction elements of the underlying vector image.

[N-Dekker]

Thanks for your clear explanation, @hjmjohnson Unfortunately I think my knowledge and understanding of MatrixOffsetTransformBase derived transforms just isn't sufficient yet 🤷

FixedParameters should never change after being chosen

MatrixOffsetTransformBase::SetIdentity() does zero-fill the m_Center. The values of m_Center are also part of FixedParameters, right? When you restore the previous values of FixedParameters, but you leave m_Center zero-filled, FixedParameters and m_Center will get out of sync, right? Isn't that a problem?

BTW, I won't stop this PR, I'm just trying to understand, because it seems a bit tricky to me!

[N-Dekker]

Could you possibly still explain where in the original SetIdentity() implementation the FixedParameters gets modified? Because I don't see anything like m_FixedParameters = someNewValue here: https://github.com/InsightSoftwareConsortium/ITK/blob/13ccff6af85522d77aa0e5e19fa929988a9de49d/Modules/Core/Transform/include/itkMatrixOffsetTransformBase.hxx#L96-L107

[hjmjohnson]

@N-Dekker The problem (at least for the Euler3DTransform) is explicitly that the m_Center is being filled with all zeros. Initially I tried removing filling the center with all zeros, but that caused a family of "Scale*Transforms" to start failing regression tests. The issue, I think, is that the behavior of the m_Center and m_Offset parameters can be manipulated by "SetFixedParameters" of the child classes.

My solution works under the principle that transforms can be properly configured with only "SetParamters" and "SetFixedParameters" from the child classes (as is done in the TransformIO mechansims). Under this known behavior of all transforms, I concluded that restoring the fixed parameters would set the state of the class correct for whatever weird behavior a child class may choose to do.

Well. ...... that was my rational for this approach which a) Passes all regression tests, and b) fixes the nasty conceptual bug I ran into when using the Eurler3DTransform with SetIdentity.

[N-Dekker]

Thanks again Hans. I just did a rebuild of the latest master branch, having locally removed the questionable m_Center zero-filling:

https://github.com/InsightSoftwareConsortium/ITK/blob/13ccff6af85522d77aa0e5e19fa929988a9de49d/Modules/Core/Transform/include/itkMatrixOffsetTransformBase.hxx#L102

I'll try the tests in a moment...

Update: as far as I can see, the Scale*Transform tests are passing successfully on my PC (VS2019 Debug), after having locally removed the m_Center zero-filling:

1>          Start 2931: itkScaleSkewVersor3DTransformTest
1>2932/3033 Test #2931: itkScaleSkewVersor3DTransformTest ......................................................................   Passed    0.36 sec
1>          Start 2932: itkComposeScaleSkewVersor3DTransformTest
1>2933/3033 Test #2932: itkComposeScaleSkewVersor3DTransformTest ...............................................................   Passed    0.30 sec
...
1>          Start 2937: itkScaleVersor3DTransformTest
1>2938/3033 Test #2937: itkScaleVersor3DTransformTest ..........................................................................   Passed    0.27 sec
...
1>          Start 2945: itkScaleLogarithmicTransformTest
1>2946/3033 Test #2945: itkScaleLogarithmicTransformTest .......................................................................   Passed    0.29 sec
..
1>          Start 2947: itkScaleTransformTest
1>2948/3033 Test #2947: itkScaleTransformTest ..................................................................................   Passed    0.28 sec

Which specific regression failures did you get? Do you think they were major failures, or possibly just rounding errors?

[N-Dekker]

I think this PR is ready for merge, but I would like to see one more approval! Especially because there is a tiny little chance that some user may experience slightly different behavior, because with this PR, MatrixOffsetTransformBase::SetIdentity() no longer resets the center. Again, I think the PR is entirely correct, but one more approval would be nice!