BUG: added 2 missing lines

seanm commented 1 month ago

@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 commented 1 month ago

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

dzenanz commented 1 month ago

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

thewtex commented 4 weeks ago

@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 commented 4 weeks ago

And it may not be correct following the discussion around:

https://github.com/InsightSoftwareConsortium/ITK/pull/4485#issuecomment-1975185951

seanm commented 4 weeks ago

And it may not be correct

So shall I just close/discard this?

dzenanz commented 4 weeks ago

Ideally, Hans and/or Niels will review this 😄

N-Dekker commented 4 weeks ago

Ideally, Hans and/or Niels will review this

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

N-Dekker commented 4 weeks ago

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: https://github.com/InsightSoftwareConsortium/ITK/blob/07f8459bdf913647dab52108d8ff284d05eb4fe4/Modules/Core/Transform/include/itkMatrixOffsetTransformBase.hxx#L492-L497

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 commented 4 weeks ago

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)