-
-
Notifications
You must be signed in to change notification settings - Fork 661
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
(Not ready) Simpler TransformPhysicalPointToContinuousIndex(point)
overloads
#4002
base: master
Are you sure you want to change the base?
Conversation
ba80ce2
to
a58b507
Compare
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 love it!
a58b507
to
2abdcb5
Compare
[[nodiscard]] ContinuousIndex<SpacePrecisionType, VImageDimension> | ||
TransformPhysicalPointToContinuousIndex(const PointType & point) const | ||
{ | ||
return TransformPhysicalPointToContinuousIndex<SpacePrecisionType>(point); | ||
} |
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.
Would it be useful to have two of such (simple, non-templated) overloads? One of float
and one of double
? Specifically:
ContinuousIndex<float, VImageDimension>
TransformPhysicalPointToContinuousIndex(const Point<float, VImageDimension> &) const;
ContinuousIndex<double, VImageDimension>
TransformPhysicalPointToContinuousIndex(const Point<double, VImageDimension> &) const;
So one from Point<float, N>
to ContinuousIndex<float, N>
, and one from Point<double, N>
to ContinuousIndex<double, N>
? Overload resolution would then automatically choose the right one, no need for the user to specify template arguments. Right?
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.
The float one is rarely used, I believe. If someone wants an unusual thing, they can spell out the template parameters.
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'm wondering, could all those image->template TransformPhysicalPointToContinuousIndex<ContinuousValueType>(point)
calls inside ITK itself then replaced by simple non-templated image->TransformPhysicalPointToContinuousIndex(point)
calls?
TransformPhysicalPointToContinuousIndex(point)
overloadTransformPhysicalPointToContinuousIndex(point)
overloads
Added missing `template` keywords to TransformPhysicalPointToContinuousIndex calls in Nonunit/Review tests. Addressed compile errors from Ubuntu-22.04-gcc11.2-TBB-Debug saying: > error: expected primary-expression before '>' token These compile errors were introduced by pull request InsightSoftwareConsortium#4000 commit 7cda5ba "COMP: Fix TransformPhysicalPointToContinuousIndex `nodiscard` warnings"
Added non-template overloads, which allow the user to write: index = image->TransformPhysicalPointToContinuousIndex(point); Instead of calling one of the overload that was added with ITK 5.0, for example: index = image->template TransformPhysicalPointToContinuousIndex<typename ContinuousIndexType::ValueType>(point);
2abdcb5
to
f5950a8
Compare
Did replace `template TransformPhysicalPointToContinuousIndex<.+>` calls with `TransformPhysicalPointToContinuousIndex`, using regular expressions. Only included `TransformPhysicalPointToContinuousIndex` calls for which both template parameters (for `TIndexRep` and `TCoordRep`) were equal to `itk::SpacePrecisionType`. Excluded unit tests from tkImageBaseGTest.cxx and itkImageAdaptorGTest.cxx as they intentionally called the member function _template_.
f5950a8
to
983bf81
Compare
Is this ready for merging? It would fix a lot of compile errors on the dashboard. |
Is ITK/Modules/Core/Common/include/itkFloatTypes.h Lines 26 to 31 in 1f8c0f8
Maybe it's just me, but I got a lot of compile errors when I try to compile the current master with I'm asking here because |
|
If we can't have a simple, easy to use alternative to the old " |
I think that as long as ITK supports |
I think we should test the ITK_USE_FLOAT_SPACE_PRECISION, I could think of a customer who may use FLOAT_SPACE_PRECISION for the purpose of improving computational speed for their algorithm. Removing this option MAY cause that customer great pain. The above scenario is purely hypothetical .... just pointing out that removal may have unintended consequences for customers. |
TransformPhysicalPointToContinuousIndex(point)
overloadsTransformPhysicalPointToContinuousIndex(point)
overloads
Added non-template overloads, which allow the user to write:
Instead of calling one of the overload that was added with ITK 5.0, for example:
I assume that usually (or at least in most common use cases), the user just wants TransformPhysicalPointToContinuousIndex to just yield a
ContinuousIndex<SpacePrecisionType, N>
(withSpacePrecisionType
=double
, by default). So in most use cases, the ValueType of ContinuousIndex does not need to be a template argument of this function. Is that OK?