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

ENH: Declare front() and back() of Index, Offset, and Size constexpr #4605

Conversation

N-Dekker
Copy link
Contributor

Follow-up to pull request #3236 commit 47bce26
"ENH: Declare begin(), end() of FixedArray, Index, Offset, Size constexpr"

Follow-up to pull request InsightSoftwareConsortium#3236
commit 47bce26
"ENH: Declare begin(), end() of FixedArray, Index, Offset, Size constexpr"
@github-actions github-actions bot added type:Infrastructure Infrastructure/ecosystem related changes, such as CMake or buildbots type:Enhancement Improvement of existing methods or implementation type:Testing Ensure that the purpose of a class is met/the results on a wide set of test cases are correct area:Core Issues affecting the Core module labels Apr 22, 2024
@N-Dekker
Copy link
Contributor Author

N-Dekker commented Apr 23, 2024

@thewtex Would this PR still be feasible for v5.4.0? It just adds constexpr keywords to front() and back() member functions.


FYI, Adding constexpr to itk::Index::back() would be a stepping stone towards evaluating the OffsetToIndexTable of BSplineInterpolationWeightFunction at compile-time, instead of at run-time:

const TableType m_OffsetToIndexTable{ [] {

Which is likely to speed up BSplineInterpolationWeightFunction::Evaluate:

BSplineInterpolationWeightFunction<TCoordRep, VSpaceDimension, VSplineOrder>::Evaluate(
const ContinuousIndexType & index,
WeightsType & weights,
IndexType & startIndex) const

@blowekamp
Copy link
Member

@thewtex Would this PR still be feasible for v5.4.0? It just adds constexpr keywords to front() and back() member functions.

Out of my curiosity, is this for a particular issue? Has it given any measurable performance benefit?

@N-Dekker
Copy link
Contributor Author

Out of my curiosity, is this for a particular issue? Has it given any measurable performance benefit?

This PR would pave the way to adding constexpr to itk::Index iteration, by itk::IndexRange. Which would allow creating the OffsetToIndexTable of BSplineInterpolationWeightFunction at compile-time, instead of at run-time. I haven't yet been able to benchmark, but it may certainly benefit its performance.

Note that itk::Index<N> is similar to std::array<itk::IndexValueType, N>, whose front() and back() are also marked "constexpr": https://en.cppreference.com/w/cpp/container/array/back

@N-Dekker N-Dekker marked this pull request as ready for review April 24, 2024 10:16
Copy link
Member

@thewtex thewtex left a comment

Choose a reason for hiding this comment

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

@N-Dekker this is awesome!

I think we should save it for ITK 6 to avoid any unforeseen issues.

@N-Dekker N-Dekker marked this pull request as draft April 24, 2024 18:07
@N-Dekker
Copy link
Contributor Author

I think we should save it for ITK 6 to avoid any unforeseen issues.

@thewtex OK, thanks. Marked "draft" to avoid that this PR might accidentally be merged before the release of ITK 5.4!

@N-Dekker N-Dekker marked this pull request as ready for review May 21, 2024 08:06
@N-Dekker
Copy link
Contributor Author

I think this pull request may also be merged now, as v5.4.0 has been tagged (#4603 (comment))

@thewtex thewtex merged commit c4d0d1b into InsightSoftwareConsortium:master May 21, 2024
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:Core Issues affecting the Core module type:Enhancement Improvement of existing methods or implementation type:Infrastructure Infrastructure/ecosystem related changes, such as CMake or buildbots type:Testing Ensure that the purpose of a class is met/the results on a wide set of test cases are correct
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants