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

• No API changes were made (or the changes have been approved)
• No major design changes were made (or the changes have been approved)
• 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:

ITK/Modules/Core/Transform/include/itkMatrixOffsetTransformBase.hxx

Lines 96 to 107 in 13ccff6

	MatrixOffsetTransformBase<TParametersValueType, VInputDimension, VOutputDimension>::SetIdentity()
	{
	m_Matrix.SetIdentity();
	m_MatrixMTime.Modified();
	m_Offset.Fill(OutputVectorValueType{});
	m_Translation.Fill(OutputVectorValueType{});
	m_Center.Fill(InputPointValueType{});
	m_Singular = false;
	m_InverseMatrix.SetIdentity();
	m_InverseMatrixMTime = m_MatrixMTime;
	this->Modified();
	}

[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:

ITK/Modules/Core/Transform/include/itkMatrixOffsetTransformBase.hxx

Line 102 in 13ccff6

m_Center.Fill(InputPointValueType{});

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]

Thanks very much for giving this approach a try, Hans! I think it's the right way to go. As far as I can see, the Scale*Transform tests still pass 👍 (Right? Please check!) Only itkSimilarity2DTransformTest appears to fail, but I think itkSimilarity2DTransformTest itself is wrong! I'll have another look.

---

I see, itkSimilarity2DTransformTest assumes that transform->SetIdentity() also resets the center. If we agree that that assumption should be dropped (the main purpose of your PR), the test should just manually reset the center. Specifically here, around line 159:

ITK/Modules/Core/Transform/test/itkSimilarity2DTransformTest.cxx

Lines 158 to 159 in cd49925

	// 15 degrees in radians
	transform->SetIdentity();

The test fails because it tries to do the 15 degrees rotations from the wrong center. It will pass when a transform->SetCenter({}) is added, either before or after transform->SetIdentity().

Hope that helps!

[hjmjohnson]

@N-Dekker Thanks. I won't be able to spend time digging into this until after next week. I appreciate any assistance I can get.

2877 - itkSimilarity2DTransformTest (Failed)

[N-Dekker]

Technically, just adding transform->SetCenter({}) to line 159 of itkSimilarity2DTransformTest.cxx should make the CI happy.

Conceptually, people should no longer assume that transform->SetIdentity() "automagically" resets the center. It's a (potentially) breaking change. But I think it's a correct one.

[N-Dekker]

Thanks very much, Hans! 👍

[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!