STYLE: Use image->TransformPhysicalPointToIndex(point), returning index

[N-Dekker]

Replaced image->TransformPhysicalPointToIndex(point, index) calls by
image->TransformPhysicalPointToIndex(point), which returns the index.

The TransformPhysicalPointToIndex(point) overload was introduced with
ITK 5.0.0. It is slightly faster than the original overload, as it does
not figure out whether or not the point is inside the image. However,
this commit is mostly intended to increase code readability.

[blowekamp]

For cases like this without a new left hand side variable, do you think an extra copy operation is being introduced?

[N-Dekker]

Thanks for your question @blowekamp I don't think returning an itk::Index object and copying this object into a variable (ind) is in any way more expensive than modifying the ind inside the function via pass-by-reference. itk::Index objects have cheap, fast copy-semantics (no dynamic memory allocation involved).

[blowekamp]

I am quite curious if there is a performance difference. My intution ( experience) is that doing it in-place would be slightly better unless there is something similar to C++ RVO. It would be quite good to have concrete evidence of the performance difference.

Would you like to do the profiling?

[N-Dekker]

I just observed that the new (ITK5) TransformPhysicalPointToIndex(point) overload is more than 20% faster than the original TransformPhysicalPointToIndex(point, index). 😄

I also looked specifically at SpatialObjectToImageStatisticsCalculator::Update(). But honestly I could not observe a significant performance effect of this commit on calculator->Update(). Probably because the Update() does so many other things than just TransformPhysicalPointToIndex.

[blowekamp]

Nice work! It is always good to do this type of analysis to determine "best practices" and recommendations as we continue to move forward. Here we can say that the new method is recommended when the isInBounds is not needed.

[N-Dekker]

For the record, support for image->TransformPhysicalPointToIndex(point) was introduced with pull request #868, commit 65233e3 ENH: Convenience overloads for ImageBase Transform member functions

[blowekamp]

As a side note, we do need to consider adding a similar return value signatures for TransformPhysicalPointToContinuousIndex and TransformPhysicalPointToIndex to maintain a consistent interface.

[hjmjohnson]

As a side note, we do need to consider adding a similar return value signatures for TransformPhysicalPointToContinuousIndex and TransformPhysicalPointToIndex to maintain a consistent interface.

@blowekamp Can you make an issue for this observation?

[blowekamp]

Outstanding! Looks like the methods are already there. It is just the Doxygen is out of date and not showing the new interface 👀 Great job again @N-Dekker

[N-Dekker]

Thanks for your approvals, Bradley and Hans! Thanks for the merge!

It is just the Doxygen is out of date

Indeed, Bradley, it still says "Generated on Sun Mar 24 2019", at https://itk.org/Doxygen/html/classitk_1_1ImageBase.html Would be nice to have it updated, now that ITK 5.0.0 is out! Is that something Matt @thewtex can fix?

[dzenanz]

Matt is still on vacation (he should be back next week), and Hans is working towards getting Doxygen to a properly building state.

[N-Dekker]

Matt is still on vacation (he should be back next week), and Hans is working towards getting Doxygen to a properly building state.

Thanks @dzenanz Unfortunately https://itk.org/Doxygen/html/index.html still says "Generated on Sun Mar 24 2019". It's really out of date now, compared to the final release. Hope you can still fix it, Matt @thewtex or Hans @hjmjohnson ?

[thewtex]

Thanks for the updates @N-Dekker !

Yes, the Doxygen updates are definitely on the todo list :-). I believe @hjmjohnson has made a few updates to the Doxygen configuration, and I am working on a nightly build running on a new system.