Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixed parameter preservation after identity #4485

Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -99,7 +99,9 @@ MatrixOffsetTransformBase<TParametersValueType, VInputDimension, VOutputDimensio
m_MatrixMTime.Modified();
m_Offset.Fill(OutputVectorValueType{});
m_Translation.Fill(OutputVectorValueType{});
m_Center.Fill(InputPointValueType{});
// Fixed parameters must be preserved when setting the transform to identity
// the center is part of the stationary fixed parameters
// and should not be modified by SetIdentity. m_Center.Fill(InputPointValueType{});
Comment on lines -102 to +104
Copy link
Contributor

@N-Dekker N-Dekker Mar 6, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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)

Copy link
Contributor

@N-Dekker N-Dekker Mar 6, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

m_Singular = false;
m_InverseMatrix.SetIdentity();
m_InverseMatrixMTime = m_MatrixMTime;
15 changes: 15 additions & 0 deletions Modules/Core/Transform/test/itkEuler3DTransformGTest.cxx
Original file line number Diff line number Diff line change
@@ -33,3 +33,18 @@ TEST(Euler3DTransform, SetFixedParametersThrowsWhenSizeIsLessThanInputSpaceDimen
ASSERT_THROW(transform->SetFixedParameters(fixedParameters), itk::ExceptionObject);
}
}

TEST(Euler3DTransform, TestSetGetCenterAfterSetIdentity)
hjmjohnson marked this conversation as resolved.
Show resolved Hide resolved
{
using EulerTransformType = itk::Euler3DTransform<>;

// Testing preservation of the center of rotation
auto eulerTransformWithCenter = EulerTransformType::New();
const itk::Point<double, 3> centerOfRotation({ 200, 400, 300 });
eulerTransformWithCenter->SetCenter(centerOfRotation);
EXPECT_EQ(eulerTransformWithCenter->GetCenter(), centerOfRotation);
eulerTransformWithCenter->SetIdentity();
// The center of rotation should be preserved when the transform is set to identity.
// Reseting a transform to identity should not affect the FixedParameters.
EXPECT_EQ(eulerTransformWithCenter->GetCenter(), centerOfRotation);
}
Original file line number Diff line number Diff line change
@@ -156,6 +156,7 @@ itkSimilarity2DTransformTest(int, char *[])
}

// 15 degrees in radians
transform->SetCenter({}); // Explicitly reset center to default values
transform->SetIdentity();
const double angle = 15.0 * std::atan(1.0f) / 45.0;
const double sinth = std::sin(angle);