-
Notifications
You must be signed in to change notification settings - Fork 119
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
Sync Transform WriteToFile with CreateTransformParametersMap #385
Conversation
@@ -159,9 +159,6 @@ DeformationFieldTransform<TElastix>::WriteToFile(const ParametersType & param) c | |||
|
|||
typedef itk::ChangeInformationImageFilter<DeformationFieldType> ChangeInfoFilterType; | |||
|
|||
/** Add some DeformationFieldTransform specific lines. */ | |||
xout["transpar"] << std::endl << "// DeformationFieldTransform specific" << std::endl; | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please check that DeformationFieldTransform::WriteToFile
cannot be entirely removed!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
indeed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mstaring So is it OK to still have some parameters written to file that are not in the parameter map?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or would it be an option to add the parameter "DeformationFieldFileName" (which is now only written to file) to the map as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess ideally you would push the whole deformation field to that map. Or at least the filename as you say, so that one can read it in again via disk. Lets discuss over sprint.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By the way, the "making" of the DeformationFieldFileName throws an exception (std::out_of_range
) inside DeformationFieldTransform::WriteToFile
when the current TransformParameterFileName is not specified. Would it be OK to suppress (or avoid) the warning and just leave DeformationFieldFileName as an empty string instead? https://github.com/SuperElastix/elastix/blob/5.0.1/Components/Transforms/DeformationFieldTransform/elxDeformationFieldTransform.hxx#L180-L182
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could assume empty string for lastpart, when TransformParameterFileName is empty.
normalizeString = "true"; | ||
} | ||
xout["transpar"] << "(NormalizeCombinationWeights \"" << normalizeString << "\" )" << std::endl; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please check that WeightedCombinationTransform::WriteToFile
cannot be entirely removed!
Note: the WeightedCombinationTransform::WriteToFile
override may be removed when "SubTransforms" is added to the parameter map!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Update: "SubTransforms" is now added to the WeightedCombinationTransform
parameter map (pull request #391 commit ae9c0fd) And this pull request is adjusted to simply remove the WeightedCombinationTransform::WriteToFile
override. 😃 With this force-push
a932d6c
to
992e891
Compare
@@ -1022,54 +1022,6 @@ BSplineTransformWithDiffusion<TElastix>::WriteToFile(const ParametersType & para | |||
xl::xout["error"] << excp << std::endl; | |||
} | |||
|
|||
/** Get the GridSize, GridIndex, GridSpacing and |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please check that BSplineTransformWithDiffusion::WriteToFile
cannot be entirely removed!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
indeed
992e891
to
36cd93a
Compare
Note that there are still two TODO's from related pull request #379 "Add missing parameters to transform parameter maps" |
lgtm |
Anticipating pull request #385 "Sync Transform WriteToFile with CreateTransformParametersMap".
Anticipating pull request #385 "Sync Transform WriteToFile with CreateTransformParametersMap".
f577e26
to
e837747
Compare
Added "MultiBSplineTransformWithNormalLabels" to the transform parameter map of `MultiBSplineTransformWithNormal`, addressing a "TODO" comment. Anticipating pull request #385 "Sync Transform WriteToFile with CreateTransformParametersMap". Also improved the `Test_CreateTransformParametersMap_for_default_transform()` unit test function for `MultiBSplineTransformWithNormal`. Note that this change is not yet completely unit tested, because `MultiBSplineTransformWithNormal::BeforeAll()` throws an exception, saying "ERROR: Missing -labels argument!".
Added "MultiBSplineTransformWithNormalLabels" to the transform parameter map of `MultiBSplineTransformWithNormal`, addressing a "TODO" comment. Anticipating pull request #385 "Sync Transform WriteToFile with CreateTransformParametersMap". Also improved the `Test_CreateTransformParametersMap_for_default_transform()` unit test function for `MultiBSplineTransformWithNormal`. Note that this change is not yet completely unit tested, because `MultiBSplineTransformWithNormal::BeforeAll()` throws an exception, saying "ERROR: Missing -labels argument!".
Synchronized the string representation of double floating point values of transform parameter maps (produced by elastix `CreateTransformParametersMap`) with ITK's .tfm file format (as written by `itk::TransformFileWriter`). Increased the floating point precision from 6 to 16. ITK's `itk::NumberToString` uses a lossless floating point conversion from the Google double-conversion library, https://github.com/google/double-conversion Part of issue #383 "Reconsider string representation of floating points for parameter files and parameter maps"
Added a utility function, which is intended to estimate whether a transform parameter value should be "double quoted", when elastix `Transform::WriteToFile` writes such a value to a TransformParameters .txt file. (A numeric parameter value should not be double quoted.)
Adjusted `TransformBase::WriteToFile` by using the parameter map created by `CreateTransformParametersMap`. Removed 14 out of 16 `WriteToFile` overrides. Ensured that the transform parameter txt file is kept in sync with the corresponding the parameter map. Thereby also adjusted the floating point format for values written to file, as discussed at issue #383, "Reconsider string representation of floating points for parameter files and parameter maps", #383 Addresses issue #217 'the "Origin" precision in TransformParameters is not high enough', reported by GouZi2019, January 5, 2020: #217
e837747
to
1c5d2a0
Compare
This elastix parameter was mainly intended to allow the user to specify the precision of floating point parameters in the transform parameter file. However, with pull request #385 commit af382ee "ENH: Sync Transform WriteToFile with CreateTransformParametersMap" (related to issue #383 "Reconsider string representation of floating points for parameter files and parameter maps"), this is no longer relevant. Floating point values are now written to transform parameter files in a lossless way.
This elastix parameter was mainly intended to allow the user to specify the precision of floating point parameters in the transform parameter file. However, with pull request #385 commit af382ee "ENH: Sync Transform WriteToFile with CreateTransformParametersMap" (related to issue #383 "Reconsider string representation of floating points for parameter files and parameter maps"), this is no longer relevant. Floating point values are now written to transform parameter files in a lossless way.
No description provided.