BUG: added 2 missing lines

[seanm]

No description provided.

[seanm]

@thewtex here are the 2 missing lines you requested in #3037. I have no idea what this does or fixes though, so could you propose a proper commit message and I'll amend the commit...

[dzenanz]

I remember there was some discussion about this not long ago. @N-Dekker @hjmjohnson

[dzenanz]

This is the PR where that somewhat related discussion happened: #4485.

[thewtex]

@seanm per #3037 , those lines are meant to go at the start of

template <typename TScalar, unsigned int NInputDimensions,
          unsigned int NOutputDimensions>
MatrixOffsetTransformBase<TScalar, NInputDimensions, NOutputDimensions>
::MatrixOffsetTransformBase(const MatrixType & matrix, const OutputVectorType & offset)
{
// here

[thewtex]

And it may not be correct following the discussion around:

#4485 (comment)

[seanm]

And it may not be correct

So shall I just close/discard this?

[dzenanz]

Ideally, Hans and/or Niels will review this 😄

[N-Dekker]

Ideally, Hans and/or Niels will review this

OK, thanks @dzenanz I'll have a look! (I'm back 😃!)

[N-Dekker]

I'm still looking for an actual relevant use case for the MatrixOffsetTransformBase(const MatrixType &, const OutputVectorType &) constructor. It's protected, and it isn't being used by MatrixOffsetTransformBase::New(), obviously. Is it being tested, directly or indirectly?

---

Update: It appears that this specific constructor has no code coverage at all:
Coverage started on Tuesday, June 04 2024
Coverage for ./Modules/Core/Transform/include/itkMatrixOffsetTransformBase.hxx
Which makes me wonder if this constructor is very useful at all 🤷

Anyway, I guess your pull request would fix a (hypothetical?) crash, when a transform would be constructed by this specific constructor, and then transform->GetFixedParameters() would be called. Because GetFixedParameters() assumes that m_FixedParameters has at least VInputDimension elements:

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

Lines 492 to 497 in 07f8459

	MatrixOffsetTransformBase<TParametersValueType, VInputDimension, VOutputDimension>::GetFixedParameters() const
	{
	for (unsigned int i = 0; i < VInputDimension; ++i)
	{
	this->m_FixedParameters[i] = this->m_Center[i];
	}

So the proposed fix looks OK to me, although it should still have a unit test. And I still wonder if it's very useful to keep maintaining this constructor, if it is unused.

[N-Dekker]

I think the unit test with this PR might be as follows:

TEST(MatrixOffsetTransformBase, CreateWithMatrixAndOffset)
{
  class DerivedTransform : public itk::MatrixOffsetTransformBase<>
  {
  public:
    ITK_DISALLOW_COPY_AND_MOVE(DerivedTransform);

    static auto
    Create()
    {
      // Indirectly call the `MatrixOffsetTransformBase(const MatrixType &, const OutputVectorType &)` constructor.
      const itk::SmartPointer<DerivedTransform> ptr = new DerivedTransform(MatrixType(), OutputVectorType());
      ptr->UnRegister();
      return ptr;
    }

  private:
    // Inherit the constructors of MatrixOffsetTransformBase:
    using itk::MatrixOffsetTransformBase<>::MatrixOffsetTransformBase;
  };

  const auto                                  transform = DerivedTransform::Create();
  const DerivedTransform::FixedParametersType expectedFixedParameters(DerivedTransform::InputSpaceDimension, 0);

  EXPECT_EQ(transform->GetFixedParameters(), expectedFixedParameters);
}

The last line is the essential one here: EXPECT_EQ(transform->GetFixedParameters(), expectedFixedParameters) checks that m_FixedParameters has the expected number of zero-values. But moreover, it checks that GetFixedParameters() does not crash! Would that be helpful to you? (The test could be added to Core/Transform/test/itkMatrixOffsetTransformBaseGTest.cxx)

[hjmjohnson]

@N-Dekker Added test that you suggested.

[thewtex]

🙌 💯

@seanm @N-Dekker @hjmjohnson awesome teamwork!

[seanm]

Would be good is someone could reword my commit message to explain what this change actually is. :)

[N-Dekker]

Honestly, if this specific MatrixOffsetTransformBase(const MatrixType &, const OutputVectorType &) constructor isn't really useful, I would rather see it being removed. Would be willing to sacrifice the beautiful 😸 unit test that I proposed, with this PR.

[hjmjohnson]

\azp ITK.Linux

[hjmjohnson]

/azp ITK.Linux

[N-Dekker]

@hjmjohnson There are a few similar unused constructors in derived classes (11!) that should also be deprecated, when MatrixOffsetTransformBase(const MatrixType &, const OutputVectorType &) is deprecated:

AffineTransform(const MatrixType & matrix, const OutputVectorType & offset);
ComposeScaleSkewVersor3DTransform(const MatrixType & matrix, const OutputVectorType & offset);
FixedCenterOfRotationAffineTransform(const MatrixType & matrix, const OutputVectorType & offset);
QuaternionRigidTransform(const MatrixType & matrix, const OutputVectorType & offset);
Rigid3DTransform(const MatrixType & matrix, const OutputVectorType & offset);
ScalableAffineTransform(const MatrixType & matrix, const OutputVectorType & offset);
ScaleSkewVersor3DTransform(const MatrixType & matrix, const OutputVectorType & offset);
ScaleVersor3DTransform(const MatrixType & matrix, const OutputVectorType & offset);
Similarity3DTransform(const MatrixType & matrix, const OutputVectorType & offset);
VersorRigid3DTransform(const MatrixType & matrix, const OutputVectorType & offset);
VersorTransform(const MatrixType & matrix, const OutputVectorType & offset);

These 11 constructors all directly or indirectly call MatrixOffsetTransformBase(const MatrixType &, const OutputVectorType &). But none of them are tested, it appears that they aren't even compiled!!! At least not at the CI. And they are all protected, so they just won't be used anywhere, I guess.

[hjmjohnson]

@N-Dekker Thanks. I have implemented your additional suggestions.

[hjmjohnson]

/azp ITK.macOS

[hjmjohnson]

/azp ITK.macOS.Python

[N-Dekker]

@hjmjohnson I see you fixed the warnings by removing the MatrixOffsetTransformBase.CreateWithMatrixAndOffset GTest that I suggested before, which is fine by me, no problem 👍