BUG: Move InputSpaceName, OutputSpaceName from Transform to Transform…

[thewtex]

…Base

These were added to support features of the OME-Zarr transform proposals. However, they need to be available in itk::TransformBase instead of itk::Transform for access in TransformIO classes.

[PranjalSahu]

LGTM

[N-Dekker]

Thanks Matt! It looks fine to me, to move those two data members from itk::Transform<TParametersValueType, VInputDimension, VOutputDimension> to itk::TransformBase. Especially because they don't depend on VInputDimension or VOutputDimension anyway. 👍 By the way, I would call it an ENH, rather than a BUG.

Of course I would have liked to see unit tests of the form:

    itk::TransformBase & transformBase = ...; // Arrange
    transformBase.SetInputSpaceName(exampleSpaceName); // Act
    EXPECT_EQ(transformBase.GetInputSpaceName(), exampleSpaceName); // Assert

Following the AAA pattern: Arrange-Act-Assert. Maybe to consider for a follow-up....?

---

Update: I see now that such unit tests are already there (albeit in the old-style test framework, rather than my favorite GoogleTest), as added by @PranjalSahu:

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

Lines 314 to 318 in 87bcabf

	transform->SetInputSpaceName("test_inputspace");
	ITK_TEST_EXPECT_EQUAL(std::string("test_inputspace"), transform->GetInputSpaceName());
	transform->SetOutputSpaceName("test_outputspace");
	ITK_TEST_EXPECT_EQUAL(std::string("test_outputspace"), transform->GetOutputSpaceName());

    transform->SetInputSpaceName("test_inputspace");
    ITK_TEST_EXPECT_EQUAL(std::string("test_inputspace"), transform->GetInputSpaceName());

    transform->SetOutputSpaceName("test_outputspace");
    ITK_TEST_EXPECT_EQUAL(std::string("test_outputspace"), transform->GetOutputSpaceName());

For this pull request, I think it would be sufficient to add itk::TransformBase:: as follows:

    transform->itk::TransformBase::SetInputSpaceName("test_inputspace");
    ITK_TEST_EXPECT_EQUAL(std::string("test_inputspace"), transform->itk::TransformBase::GetInputSpaceName());

    transform->itk::TransformBase::SetOutputSpaceName("test_outputspace");
    ITK_TEST_EXPECT_EQUAL(std::string("test_outputspace"), transform->itk::TransformBase::GetOutputSpaceName());

[N-Dekker]

One little request! Please declare those two data members private! They were originally private already!

[blowekamp]

Any consideration for putting these into the meta-data dictionary over member variables? Would we get serialization for free with any transform IO's if we did that?

[N-Dekker]

Any consideration for putting these into the meta-data dictionary over member variables?

I guess, the question could be, are these "space names" fundamental general properties of any transform, or are they very much application specific? (In the latter case, I guess it would be better to have them as meta-data dictionary entries, rather than member variables.)

Having said so, the decision was made already almost two years ago, by adding them to itk::Transform: pull request #3470 commit 0575485. So maybe it's best to just leave them as member variables now 🤷

[blowekamp]

Having said so, the decision was made already almost two years ago, by adding them to itk::Transform: pull request #3470 commit 0575485. So maybe it's best to just leave them as member variables now 🤷

They could still have setter and getter member functions even if they are in the meta-data dictionary. It is more of a question if having them in the dictionary would make serialization easier.

Yes, this question is incidental to the intent of this PR.

[thewtex]

Thanks for the reviews.

@N-Dekker I added the extra coverage requested.

Regarding metadata vs member, I think member is more appropriate in this case: this is an explicit property that we want all tooling / IO to eventually interpret and the explicit interface is helpful. In practice it is easier to work with, including for IO, when the explicit interface is available.

[N-Dekker]

Thanks for addressing my comments, Matt! Approved 👍