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

Add missing parameters to transform parameter maps #379

Merged
merged 4 commits into from
Jan 5, 2021

Conversation

N-Dekker
Copy link
Member

No description provided.

@N-Dekker
Copy link
Member Author

@ViktorvdValk Before addressing InsightSoftwareConsortium/ITKElastix#93 and #366 I want to clean up the code of those CreateTransformParametersMap member functions, with this pull request. Maybe you could have a look, especially at the second commit, "STYLE: Simplified CreateTransformParametersMap overrides" ? The essence is that each paramsMap->insert call adds a parameter (parameter name + vector of values). This pull request moves the creation of those parameter values to ToString and ToVectorOfStrings helper functions.

@N-Dekker N-Dekker linked an issue Dec 22, 2020 that may be closed by this pull request
@N-Dekker N-Dekker force-pushed the CreateTransformParametersMap branch 13 times, most recently from caeec7f to b57a9a1 Compare December 29, 2020 10:20
@N-Dekker N-Dekker force-pushed the CreateTransformParametersMap branch 3 times, most recently from b78d174 to 8ec4ad7 Compare January 2, 2021 08:23
@N-Dekker N-Dekker changed the title Simplified CreateTransformParametersMap overrides, using ToString and ToVectorOfStrings Add missing parameters to transform parameter maps Jan 4, 2021
@N-Dekker
Copy link
Member Author

N-Dekker commented Jan 4, 2021

@mstaring @stefanklein Can we possibly discuss this pull request at the sprint, this afternoon?

@N-Dekker N-Dekker marked this pull request as ready for review January 4, 2021 14:06
@mstaring
Copy link
Member

mstaring commented Jan 4, 2021

lgtm

Added three overload sets of static member functions to `BaseComponent`:

    size_t GetNumberOfElements(container)
    string ToString(value)
    vector<string> ToVectorOfStrings(container)

Including four GoogleTest unit tests.
Made `TransformBase::CreateTransformParametersMap` non-virtual. Added transform type specific parameters by overriding the added virtual `CreateDerivedTransformParametersMap` member function, which is called by `CreateTransformParametersMap`.

Reduced the amount of code duplication, by using the new `BaseComponent::ToVectorOfStrings` and `BaseComponent::ToVectorOfStrings` convenience functions. Replaced `std::map::insert` calls by `std::map::operator[]` and C++11 list initialization (using curly braces).
Added `CreateDerivedTransformParametersMap` overrides, which add missing parameters to:

 - AffineDTITransformElastix
 - AffineLogStackTransform
 - AffineLogTransformElastix
 - BSplineTransformWithDiffusion
 - DeformationFieldTransform
 - EulerStackTransform
 - MultiBSplineTransformWithNormal
 - SimilarityTransformElastix
 - SplineKernelTransform
 - TranslationStackTransform
 - WeightedCombinationTransformElastix

Addresses the following issues:

ITKElastix issue 93: "2D similarity transform parameter map doesn't keep "CenterOfRotationPoint" parameter", by Heath Patterson, 16 December, 2020.
InsightSoftwareConsortium/ITKElastix#93

SuperElastix/elastix issue 366: "AffineDTITransform does not return the CenterOfRotationPoint", by dyliu2016, 27 November, 2020
#366

Declared `TransformBase::CreateDerivedTransformParametersMap()` pure virtual (`= 0`). Added an empty override to `TranslationTransformElastix`, just to keep allowing the transform to be instantiated.

Extended `CreateTransformParametersMapForDefaultTransform` GoogleTest unit test.

Note: Some of the overrides may still be incomplete, as indicated by comments, saying "TODO If necessary, add possibly missing parameters".
@N-Dekker N-Dekker force-pushed the CreateTransformParametersMap branch from 8ec4ad7 to 07bae26 Compare January 5, 2021 17:39
Comment on lines +1087 to +1090
// TODO If necessary, add possibly missing parameters:
// - "DeformationFieldFileName" (which is written by WriteToFile).
// - "GridDirection" (as returned by RecursiveBSplineTransform).
// - "ThresholdBool", "ThresholdHU", etc. (as retrieved by ReadParameter).
Copy link
Member Author

Choose a reason for hiding this comment

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

Note, still a TODO here...

Comment on lines +719 to +720
// TODO If necessary, add possibly missing parameters:
// - "MultiBSplineTransformWithNormalLabels" (which is written by WriteToFile).
Copy link
Member Author

Choose a reason for hiding this comment

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

...and another TODO here!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

AffineDTITransform does not return the CenterOfRotationPoint
2 participants