Closed thewtex closed 1 week ago
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: https://github.com/InsightSoftwareConsortium/ITK/blob/87bcabf7f7bae4906f183952c5a43b16e99cf390/Modules/Core/Transform/test/itkTransformTest.cxx#L314-L318
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());
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?
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 05754850fdeba5971600d3628221cf293c7d4852. So maybe it's best to just leave them as member variables now 🤷
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.
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.
…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.