PERF: Adding weights caching derivative calculation to iktMeanSquaresImageToImageMetric #4982

ljm898 · 2024-11-22T02:50:25Z

Weights caching is used for derivative calculation to reduce registration time with BSplineTransforms. Currently only implemented for MeanSquaresImageToImageMetric, other metrics to be added at a later date.

PR Checklist

No API changes were made (or the changes have been approved)
No major design changes were made (or the changes have been approved)
Added test (or behavior not changed)
Updated API documentation (or API not changed)
Added license to new files (if any)
Added Python wrapping to new files (if any) as described in ITK Software Guide Section 9.5
Added ITK examples for all new major features (if any)

Refer to the ITK Software Guide for
further development details if necessary.

github-actions

Thank you for contributing a pull request! 🙏

Welcome to the ITK community! 🤗👋☀️

We are glad you are here and appreciate your contribution. Please keep in mind our community participation guidelines. 📜
More support and guidance on the contribution process can be found in our contributing guide. 📖

This is an automatic message. Allow for time for the ITK community to be able to read the pull request and comment
on it.

hjmjohnson · 2024-11-23T16:39:18Z

@ljm898 You may have missed some modifications:

Modules/Registration/Common/include/itkMeanSquaresImageToImageMetric.hxx:169:34: error: use of undeclared identifier 'm_NumBSplineWeights'

https://open.cdash.org/viewBuildError.php?buildid=10043237

The code is not compiling for this PR.

ljm898 · 2024-11-24T21:30:30Z

Thanks in advance for your patience! Not sure why it worked on my pc when I tested it. From context and the error, it looks like it was missing a this-> at those lines. Hopefully that fixes it.

hjmjohnson

Please make this a single commit on top of the current master branch. Having two commits for 1 change makes it very hard to evaluate the change.

Recommended code:

  if (this->m_BSplineTransform && this->m_UseCachingOfBSplineWeights)
  {
    // using pre-computed weights and indexes to calculate only non zero elements of the derivative
    for (unsigned int w = 0; w < this->m_NumBSplineWeights; ++w)
    {
      const auto precomputedIndex = this->m_BSplineTransformIndicesArray[fixedImageSample][w];
      const auto precomputedWeight = this->m_BSplineTransformWeightsArray[fixedImageSample][w];
      for (unsigned int dim = 0; dim < MovingImageDimension; ++dim)
      {
        const int par = precomputedIndex + this->m_BSplineParametersOffset[dim];
        //threadS.m_MSEDerivative[par] += 2.0 * diff * precomputedWeight * movingImageGradientValue[dim];
        //Need to verify that a test fails with wrong answer below.
        threadS.m_MSEDerivative[par] += 99.0 * diff * precomputedWeight * movingImageGradientValue[dim];
      }
    }
  }
  else
  {
...

[ ] - Need to ensure that a test fails with a bad computation in the caching component.

hjmjohnson · 2024-12-03T01:50:31Z

Modules/Registration/Common/include/itkMeanSquaresImageToImageMetric.hxx

-  // Jacobian should be evaluated at the unmapped (fixed image) point.
-  transform->ComputeJacobianWithRespectToParameters(fixedImagePoint, threadS.m_Jacobian);
-  for (unsigned int par = 0; par < this->m_NumberOfParameters; ++par)
+  if (this->m_TransformIsBSpline && this->m_UseCachingOfBSplineWeights)


this->m_TransformIsBSpline is deprecated.

\deprecated `m_TransformIsBSpline` is intended to be removed, in the future. Please use `m_BSplineTransform`

hjmjohnson · 2024-12-03T01:51:34Z

Modules/Registration/Common/include/itkMeanSquaresImageToImageMetric.hxx

+      for (unsigned int dim = 0; dim < MovingImageDimension; ++dim)
+      {
+        int par = this->m_BSplineTransformIndicesArray[fixedImageSample][k] + this->m_BSplineParametersOffset[dim];
+        threadS.m_MSEDerivative[par] += 2.0 * diff * this->m_BSplineTransformWeightsArray[fixedImageSample][k] * movingImageGradientValue[dim];


I changed line 174 to
threadS.m_MSEDerivative[par] = 99;

and no tests failed.

We need to have a tests that executes this code before it can be added to the toolkit. Currently the code is never run by any tests.

Test has been added to itkMeanSquaresImageMetricTest.cxx that checks consistency between derivative computed omitting versus using weights caching. Tested on my end, can confirm it fails using 99.0 * ... and passes using 2.0 * ...

I don't see the test addition. Maybe you forgot to push the change, or pushed it improperly?

@ljm898 We are looking forward to seeing these changes :).

Looks like the code didn't push properly. The test added to itkMeanSquaresImageMetricTest.cxx is now visible - thanks for your patience!

ljm898 · 2024-12-05T04:38:41Z

Thank you for your review, I very much appreciate your time. I will get to this in the coming week.

hjmjohnson · 2024-12-08T13:59:31Z

@ljm898 thank you!

Your efforts are greatly appreciated.

hjmjohnson · 2024-12-12T16:59:45Z

Modules/Registration/Common/include/itkMeanSquaresImageToImageMetric.hxx

+      for (unsigned int dim = 0; dim < MovingImageDimension; ++dim)
+      {
+        int par = this->m_BSplineTransformIndicesArray[fixedImageSample][k] + this->m_BSplineParametersOffset[dim];
+        threadS.m_MSEDerivative[par] += 2.0 * diff * this->m_BSplineTransformWeightsArray[fixedImageSample][k] * movingImageGradientValue[dim];


@ljm898 We are looking forward to seeing these changes :).

ljm898 · 2024-12-13T01:18:19Z

Something has gone wrong on my end. GitHub sure can be fun :).

Modules/Registration/Common/include/itkMeanSquaresImageToImageMetric.hxx

Modules/Registration/Common/test/itkMeanSquaresImageMetricTest.cxx

jhlegarreta

Some style comments, but mainly the adoption of ITK_TRY_EXPECT_NO_EXCEPTION.

Modules/Registration/Common/test/itkMeanSquaresImageMetricTest.cxx

dzenanz

Mostly looks good to me.

Modules/Registration/Common/test/itkMeanSquaresImageMetricTest.cxx

…ageMetric Weights caching is used for derivative calculation to reduce registration time with BSplineTransforms. Currently only implemented for MeanSquaresImageToImageMetric, other metrics to be added at a later date.

ljm898 · 2024-12-17T03:58:02Z

Had a look into the failed/failing ITK.macOS check. itkNiftiReadWriteDirectionTest fails with code 255. I'd have expected the failed test to be independent of the suggested code changes. Any suggestions for fixing this compilation error?

N-Dekker · 2024-12-17T12:54:55Z

Had a look into the failed/failing ITK.macOS check. itkNiftiReadWriteDirectionTest fails with code 255. I'd have expected the failed test to be independent of the suggested code changes. Any suggestions for fixing this compilation error?

You may rerun the CI for this machine, by adding a comment, saying something like /azp run <machine-name>. Well... I'll just give it a try 😃

N-Dekker · 2024-12-17T12:55:03Z

/azp run ITK.macOS

dzenanz

LGTM

N-Dekker · 2024-12-17T17:57:40Z

Just for my understanding, it looks like the UseCachingOfBSplineWeights property has been around for decades already (looking at git log). Did the original developers somehow forget to use it?

In which scenario's would you suggest users to switch it on?

ljm898 · 2024-12-18T20:47:25Z

Just for my understanding, it looks like the UseCachingOfBSplineWeights property has been around for decades already (looking at git log). Did the original developers somehow forget to use it?

In which scenario's would you suggest users to switch it on?

Current versions use caching by default. AFAIK, caching was only used to compute deformation instead of the transform point method. Not sure why it wasn't originally used for derivatives - it could be the case that additional support has since been added to ITK that made it practical? I'm just stoked it works for derivatives.

github-actions bot added type:Performance Improvement in terms of compilation or execution time area:Registration Issues affecting the Registration module labels Nov 22, 2024

github-actions bot reviewed Nov 22, 2024

View reviewed changes

dzenanz requested review from hjmjohnson and N-Dekker November 22, 2024 14:44

hjmjohnson marked this pull request as draft November 23, 2024 16:39

ljm898 force-pushed the master branch from de08029 to 566092b Compare November 25, 2024 22:07

ljm898 marked this pull request as ready for review November 26, 2024 20:50

thewtex requested a review from ntustison November 29, 2024 15:28

hjmjohnson requested changes Dec 3, 2024

View reviewed changes

hjmjohnson marked this pull request as draft December 3, 2024 01:56

ljm898 force-pushed the master branch from 566092b to bfe2236 Compare December 11, 2024 20:50

ljm898 marked this pull request as ready for review December 11, 2024 23:19

hjmjohnson requested changes Dec 12, 2024

View reviewed changes

hjmjohnson mentioned this pull request Dec 12, 2024

ENH: Activate standard pre-commit hooks #5055

Merged

2 tasks

ljm898 force-pushed the master branch from bfe2236 to 95a5803 Compare December 12, 2024 23:53

github-actions bot added the type:Testing Ensure that the purpose of a class is met/the results on a wide set of test cases are correct label Dec 12, 2024

ljm898 closed this Dec 13, 2024

ljm898 force-pushed the master branch from 8414f7a to fa3e0b4 Compare December 13, 2024 01:04

github-actions bot removed type:Testing Ensure that the purpose of a class is met/the results on a wide set of test cases are correct area:Registration Issues affecting the Registration module labels Dec 13, 2024

ljm898 reopened this Dec 13, 2024

github-actions bot added type:Testing Ensure that the purpose of a class is met/the results on a wide set of test cases are correct area:Registration Issues affecting the Registration module labels Dec 13, 2024

ljm898 force-pushed the master branch from 9abd134 to 4ff060b Compare December 13, 2024 01:22

ljm898 force-pushed the master branch from 4ff060b to d3fd576 Compare December 13, 2024 01:25

N-Dekker reviewed Dec 13, 2024

View reviewed changes

Modules/Registration/Common/include/itkMeanSquaresImageToImageMetric.hxx Outdated Show resolved Hide resolved

dzenanz reviewed Dec 13, 2024

View reviewed changes

Modules/Registration/Common/test/itkMeanSquaresImageMetricTest.cxx Outdated Show resolved Hide resolved

Modules/Registration/Common/test/itkMeanSquaresImageMetricTest.cxx Outdated Show resolved Hide resolved

jhlegarreta reviewed Dec 13, 2024

View reviewed changes

ljm898 force-pushed the master branch 4 times, most recently from b0c0f7a to 5effe1a Compare December 16, 2024 04:23

dzenanz reviewed Dec 16, 2024

View reviewed changes

Modules/Registration/Common/test/itkMeanSquaresImageMetricTest.cxx Outdated Show resolved Hide resolved

ljm898 force-pushed the master branch from 5effe1a to 8ff3cbc Compare December 17, 2024 01:11

hjmjohnson self-requested a review December 17, 2024 15:11

dzenanz approved these changes Dec 17, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PERF: Adding weights caching derivative calculation to iktMeanSquaresImageToImageMetric #4982

PERF: Adding weights caching derivative calculation to iktMeanSquaresImageToImageMetric #4982

ljm898 commented Nov 22, 2024

github-actions bot left a comment

hjmjohnson commented Nov 23, 2024

ljm898 commented Nov 24, 2024

hjmjohnson left a comment

hjmjohnson Dec 3, 2024

hjmjohnson Dec 3, 2024

ljm898 Dec 11, 2024

dzenanz Dec 12, 2024

hjmjohnson Dec 12, 2024

ljm898 Dec 12, 2024 •

edited

Loading

ljm898 commented Dec 5, 2024

hjmjohnson commented Dec 8, 2024

hjmjohnson Dec 12, 2024

ljm898 commented Dec 13, 2024 •

edited

Loading

jhlegarreta left a comment

dzenanz left a comment

ljm898 commented Dec 17, 2024

N-Dekker commented Dec 17, 2024

N-Dekker commented Dec 17, 2024

dzenanz left a comment

N-Dekker commented Dec 17, 2024

ljm898 commented Dec 18, 2024

PERF: Adding weights caching derivative calculation to iktMeanSquaresImageToImageMetric #4982

Are you sure you want to change the base?

PERF: Adding weights caching derivative calculation to iktMeanSquaresImageToImageMetric #4982

Conversation

ljm898 commented Nov 22, 2024

PR Checklist

github-actions bot left a comment

Choose a reason for hiding this comment

hjmjohnson commented Nov 23, 2024

ljm898 commented Nov 24, 2024

hjmjohnson left a comment

Choose a reason for hiding this comment

hjmjohnson Dec 3, 2024

Choose a reason for hiding this comment

hjmjohnson Dec 3, 2024

Choose a reason for hiding this comment

ljm898 Dec 11, 2024

Choose a reason for hiding this comment

dzenanz Dec 12, 2024

Choose a reason for hiding this comment

hjmjohnson Dec 12, 2024

Choose a reason for hiding this comment

ljm898 Dec 12, 2024 • edited Loading

Choose a reason for hiding this comment

ljm898 commented Dec 5, 2024

hjmjohnson commented Dec 8, 2024

hjmjohnson Dec 12, 2024

Choose a reason for hiding this comment

ljm898 commented Dec 13, 2024 • edited Loading

jhlegarreta left a comment

Choose a reason for hiding this comment

dzenanz left a comment

Choose a reason for hiding this comment

ljm898 commented Dec 17, 2024

N-Dekker commented Dec 17, 2024

N-Dekker commented Dec 17, 2024

dzenanz left a comment

Choose a reason for hiding this comment

N-Dekker commented Dec 17, 2024

ljm898 commented Dec 18, 2024

ljm898 Dec 12, 2024 •

edited

Loading

ljm898 commented Dec 13, 2024 •

edited

Loading