search

SuperElastix / elastix

Official elastix repository
http://elastix.dev
Apache License 2.0
474 stars 116 forks source link

Replace `GetNumberOfWeights()`, `GetSupportSize()`, and `m_SupportSize` with compile-time equivalents (STYLE/PERF) #1099

Closed N-Dekker closed 5 months ago

N-Dekker commented 5 months ago

Replaced run-time evaluation of number of weights and support size with compile-time evaluation of those values. Might improve the run-time performance of bspline transformations and interpolations. Just marked as style improvement, because the changes are not benchmarked (yet).

stefanklein commented 5 months ago

Hey Niels, thanks. Maybe they are now the same, but in theory/future it could be different, so I think the existing implementation was more robust and generic. Best, Stefan

From: Niels Dekker @.> Sent: Wednesday, April 17, 2024 3:02 PM To: SuperElastix/elastix @.> Cc: Stefan Klein @.>; Mention @.> Subject: Re: [SuperElastix/elastix] Replace GetNumberOfWeights(), GetSupportSize(), and m_SupportSize with compile-time equivalents (STYLE/PERF) (PR #1099)

Waarschuwing: Deze e-mail is afkomstig van buiten de organisatie. Klik niet op links en open geen bijlagen, tenzij u de afzender herkent en weet dat de inhoud veilig is. Caution: This email originated from outside of the organization. Do not click links or open attachments unless you recognize the sender and know the content is safe.

@N-Dekker commented on this pull request.

In Common/Transforms/itkCyclicBSplineDeformableTransform.hxxhttps://github.com/SuperElastix/elastix/pull/1099#discussion_r1568807908:

  • const int supportLastDimSize = this->m_SupportSize.GetElement(lastDim);

@mstaringhttps://github.com/mstaring @stefankleinhttps://github.com/stefanklein The original code specifically picked the last dimension from the support size, but as far as I can see, all of its dimensions are the same: SplineOrder + 1.

- Reply to this email directly, view it on GitHubhttps://github.com/SuperElastix/elastix/pull/1099#pullrequestreview-2005985360, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AAF2LNI6VQ2AND5Q34VHTA3Y5ZXDJAVCNFSM6AAAAABGLIXG6KVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDAMBVHE4DKMZWGA. You are receiving this because you were mentioned.Message ID: @.**@.>>

N-Dekker commented 5 months ago

Maybe they are now the same, but in theory/future it could be different, so I think the existing implementation was more robust and generic.

@stefanklein @mstaring Thanks for your feedback, Stefan! Are there any plans to allow dynamically changing the "support size" of a bspline, in the near future? Otherwise I would prefer to just make the code simpler and faster for now, as in this PR. If it should eventually become more flexible in the far future, the code can still be modified, in the far future. If necessary. Is that OK to you?

Would it help if I would add a comment, saying something like:

// The original code did `supportLastDimSize = this->m_SupportSize.GetElement(lastDim)`,
// but for now, each SupportSize element just has the same value: SplineOrder + 1 
const int supportLastDimSize = VSplineOrder + 1;

Note that all tests have passed successfully.

N-Dekker commented 5 months ago

Side note, there appear two different definitions of NumberOfWeights:

I believe that only the last one is the correct one: pow(SplineOrder + 1, SpaceDimension). And the first one is wrong. OK?

The one I find suspicious, NumberOfWeights = (SplineOrder + 1) * SpaceDimension was introduced 8 years ago, commit 5a80b720269e7c6b660283d75bb9a087f6dcb678 by GetConstNumberOfWeightsHackRecursiveBSpline:

https://github.com/SuperElastix/elastix/blob/5a80b720269e7c6b660283d75bb9a087f6dcb678/src/Common/itkRecursiveBSplineInterpolationWeightFunction.h#L47-L54

It looks like the recursive step of the "hack" was missing. 🤷

stefanklein commented 5 months ago

Aren’t those in different files/functions? And both are used somewhere, right? I don’t remember exactly, but I think in the recursive bspline transform this is actually correct, because it exploits the separability of the kernel. Best, Stefan

From: Niels Dekker @.> Sent: Wednesday, April 17, 2024 5:08 PM To: SuperElastix/elastix @.> Cc: Stefan Klein @.>; Mention @.> Subject: Re: [SuperElastix/elastix] Replace GetNumberOfWeights(), GetSupportSize(), and m_SupportSize with compile-time equivalents (STYLE/PERF) (PR #1099)

Waarschuwing: Deze e-mail is afkomstig van buiten de organisatie. Klik niet op links en open geen bijlagen, tenzij u de afzender herkent en weet dat de inhoud veilig is. Caution: This email originated from outside of the organization. Do not click links or open attachments unless you recognize the sender and know the content is safe.

Side note, there appear two different definitions of NumberOfWeights:

I believe that only the last one is the correct one: pow(SplineOrder + 1, SpaceDimension). And the first one is wrong. OK?

The one I find suspicious, NumberOfWeights = (SplineOrder + 1) * SpaceDimension was introduced 8 years ago, commit 5a80b72https://github.com/SuperElastix/elastix/commit/5a80b720269e7c6b660283d75bb9a087f6dcb678 by GetConstNumberOfWeightsHackRecursiveBSpline:

https://github.com/SuperElastix/elastix/blob/5a80b720269e7c6b660283d75bb9a087f6dcb678/src/Common/itkRecursiveBSplineInterpolationWeightFunction.h#L47-L54

It looks like the recursive step of the "hack" was missing. 🤷

— Reply to this email directly, view it on GitHubhttps://github.com/SuperElastix/elastix/pull/1099#issuecomment-2061492736, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AAF2LNMMLAWRSHQEKN4ZTKDY52F47AVCNFSM6AAAAABGLIXG6KVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDANRRGQ4TENZTGY. You are receiving this because you were mentioned.Message ID: @.**@.>>

stefanklein commented 5 months ago

No concrete plans. I agree that it is not likely that it will change dynamically. I'm a bit in a doubt. The previous code was quite logical as well , right? Is the main aim to make it a static variable, instead of dynamic, or to make the code more clear? Stefan

From: Niels Dekker @.> Sent: Wednesday, April 17, 2024 3:45 PM To: SuperElastix/elastix @.> Cc: Stefan Klein @.>; Mention @.> Subject: Re: [SuperElastix/elastix] Replace GetNumberOfWeights(), GetSupportSize(), and m_SupportSize with compile-time equivalents (STYLE/PERF) (PR #1099)

Waarschuwing: Deze e-mail is afkomstig van buiten de organisatie. Klik niet op links en open geen bijlagen, tenzij u de afzender herkent en weet dat de inhoud veilig is. Caution: This email originated from outside of the organization. Do not click links or open attachments unless you recognize the sender and know the content is safe.

Maybe they are now the same, but in theory/future it could be different, so I think the existing implementation was more robust and generic.

@stefankleinhttps://github.com/stefanklein @mstaringhttps://github.com/mstaring Thanks for your feedback, Stefan! Are there any plans to allow dynamically changing the "support size" of a bspline, in the near future? Otherwise I would prefer to just make the code simpler and faster for now, as in this PR. If it should eventually become more flexible in the far future, the code can still be modified, in the far future. If necessary. Is that OK to you?

Would it help if I would add a comment, saying something like:

// The original code did supportLastDimSize = this->m_SupportSize.GetElement(lastDim),

// but for now, each SupportSize element just has the same value: SplineOrder + 1

const int supportLastDimSize = VSplineOrder + 1;

Note that all tests have passed successfully.

- Reply to this email directly, view it on GitHubhttps://github.com/SuperElastix/elastix/pull/1099#issuecomment-2061298615, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AAF2LNMOBU7ZHWO4AE2ARIDY5Z4FPAVCNFSM6AAAAABGLIXG6KVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDANRRGI4TQNRRGU. You are receiving this because you were mentioned.Message ID: @.**@.>>

N-Dekker commented 5 months ago

Aren’t those in different files/functions? And both are used somewhere, right? I don’t remember exactly, but I think in the recursive bspline transform this is actually correct, because it exploits the separability of the kernel.

Thanks Stefan, but if NumberOfWeights = (SplineOrder + 1) * SpaceDimension would have been correct, then why would Wyke Huizinga (@whuizinga) have made a "hack" to estimate its value? Referring again to https://github.com/SuperElastix/elastix/blob/5a80b720269e7c6b660283d75bb9a087f6dcb678/src/Common/itkRecursiveBSplineInterpolationWeightFunction.h#L47-L54 which did:

/** Recursive template to retrieve the number of B-spline weights at compile time. */
template< unsigned int SplineOrder, unsigned int Dimension >
class GetConstNumberOfWeightsHackRecursiveBSpline
{
public:

  itkStaticConstMacro( Value, unsigned int, ( SplineOrder + 1 ) * Dimension  );
};

and then Wyke (I assume) did:

  /** Get number of hacks. */
  typedef GetConstNumberOfWeightsHackRecursiveBSpline<
    itkGetStaticConstMacro( SplineOrder ),
    itkGetStaticConstMacro( SpaceDimension ) > GetConstNumberOfWeightsHackRecursiveBSplineType;
  itkStaticConstMacro( NumberOfWeights, unsigned int, GetConstNumberOfWeightsHackRecursiveBSplineType::Value );

It looks to me like the intention was to cleverly calculate pow(SplineOrder + 1, SpaceDimension) at compile-time, but it produces just ( SplineOrder + 1 ) * Dimension. Otherwise, why was there a hack anyway? Why not directly do:

 itkStaticConstMacro( NumberOfWeights, unsigned int, ( SplineOrder + 1 ) * Dimension  );

Anyway, maybe only Wyke knows 🤷 . In order to avoid any confusion, I would prefer to just remove RecursiveBSplineInterpolationWeightFunction::NumberOfWeights, as it is unused anyway:

whuizinga commented 5 months ago

Hi,

I am not sure anymore. It is a long time ago. But isn't space dimension a template argument that increases in the recursion? Then it would make sense to put it in the recursion. Otherwise I dont really see it either....

Cheers,

Wyke

Op wo 17 apr 2024 18:16 schreef Niels Dekker @.***>:

Aren’t those in different files/functions? And both are used somewhere, right? I don’t remember exactly, but I think in the recursive bspline transform this is actually correct, because it exploits the separability of the kernel.

Thanks Stefan, but if NumberOfWeights = (SplineOrder + 1) * SpaceDimension would have been correct, then why would Wyke Huizinga @.*** https://github.com/whuizinga) have made a "hack" to estimate its value? Referring again to https://github.com/SuperElastix/elastix/blob/5a80b720269e7c6b660283d75bb9a087f6dcb678/src/Common/itkRecursiveBSplineInterpolationWeightFunction.h#L47-L54 which did:

/* Recursive template to retrieve the number of B-spline weights at compile time. /template< unsigned int SplineOrder, unsigned int Dimension >class GetConstNumberOfWeightsHackRecursiveBSpline {public:

itkStaticConstMacro( Value, unsigned int, ( SplineOrder + 1 ) * Dimension ); };

and then Wyke (I assume) did:

/* Get number of hacks. / typedef GetConstNumberOfWeightsHackRecursiveBSpline< itkGetStaticConstMacro( SplineOrder ), itkGetStaticConstMacro( SpaceDimension ) > GetConstNumberOfWeightsHackRecursiveBSplineType; itkStaticConstMacro( NumberOfWeights, unsigned int, GetConstNumberOfWeightsHackRecursiveBSplineType::Value );

It looks to me like the intention was to cleverly calculate pow(SplineOrder

  • 1, SpaceDimension) at compile-time, but it produces just ( SplineOrder

  • 1 ) * Dimension. Otherwise, why was there a hack anyway? Why not directly do:

    itkStaticConstMacro( NumberOfWeights, unsigned int, ( SplineOrder + 1 ) * Dimension );

Anyway, maybe only Wyke knows 🤷 . In order to avoid any confusion, I would prefer to just remove RecursiveBSplineInterpolationWeightFunction::NumberOfWeights, as it is unused anyway:

— Reply to this email directly, view it on GitHub https://github.com/SuperElastix/elastix/pull/1099#issuecomment-2061692234, or unsubscribe https://github.com/notifications/unsubscribe-auth/AG2LMK5MXXKETJROKFWHNIDY52N5ZAVCNFSM6AAAAABGLIXG6KVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDANRRGY4TEMRTGQ . You are receiving this because you were mentioned.Message ID: @.***>

N-Dekker commented 5 months ago

Hi, I am not sure anymore. It is a long time ago. But isn't space dimension a template argument that increases in the recursion? Then it would make sense to put it in the recursion. Otherwise I dont really see it either.... Cheers, Wyke

Thank you very much for your reply, Wyke! Really cool to see that you still comment on this old code, after such a long time!

It's interesting to note that "the other NumberOfWeights definition", from BSplineInterpolationWeightFunctionBase, did use a slightly different "hack", at https://github.com/SuperElastix/elastix/blob/ef8a591818513661465043dd73a15e35ed449ec8/src/Common/Transforms/itkBSplineInterpolationWeightFunctionBase.h#L30-L46

 /** Recursive template to retrieve the number of Bspline weights at compile time. */ 
 template <unsigned int SplineOrder, unsigned int Dimension> 
 class GetConstNumberOfWeightsHack 
 { 
 public: 
   typedef GetConstNumberOfWeightsHack<SplineOrder, Dimension-1> OneDimensionLess; 
   itkStaticConstMacro( Value, unsigned long, (SplineOrder+1) * OneDimensionLess::Value ); 
 }; 

 /** Partial template specialization to terminate the recursive loop. */ 
 template <unsigned int SplineOrder> 
 class GetConstNumberOfWeightsHack<SplineOrder, 0> 
 { 
 public: 
   itkStaticConstMacro( Value, unsigned long, 1 ); 
 }

So that's why BSplineInterpolationWeightFunctionBase::NumberOfWeights became equal to pow(SplineOrder + 1, SpaceDimension), whereas RecursiveBSplineInterpolationWeightFunction::NumberOfWeights just became equal to (SplineOrder + 1) * SpaceDimension! Anyway, it looks like the latter one wasn't really used, so I'm now proposing to just remove the latter one: https://github.com/SuperElastix/elastix/pull/1100

whuizinga commented 5 months ago

Ok yes, the other function you mention makes more sense.

But if it is not being used, then indeed, it can be removed. Maybe it was there to validate some computations..

Best,

Wyke

Op wo 17 apr 2024 22:37 schreef Niels Dekker @.***>:

Hi, I am not sure anymore. It is a long time ago. But isn't space dimension a template argument that increases in the recursion? Then it would make sense to put it in the recursion. Otherwise I dont really see it either.... Cheers, Wyke

Thank you very much for your reply, Wyke! Really cool to see that you still comment on this old code, after such a long time!

It's interesting to note that "the other NumberOfWeights definition", from BSplineInterpolationWeightFunctionBase, did use a slightly different "hack", at https://github.com/SuperElastix/elastix/blob/ef8a591818513661465043dd73a15e35ed449ec8/src/Common/Transforms/itkBSplineInterpolationWeightFunctionBase.h#L30-L46

/* Recursive template to retrieve the number of Bspline weights at compile time. / template <unsigned int SplineOrder, unsigned int Dimension> class GetConstNumberOfWeightsHack { public: typedef GetConstNumberOfWeightsHack<SplineOrder, Dimension-1> OneDimensionLess; itkStaticConstMacro( Value, unsigned long, (SplineOrder+1) * OneDimensionLess::Value ); };

/* Partial template specialization to terminate the recursive loop. / template class GetConstNumberOfWeightsHack<SplineOrder, 0> { public: itkStaticConstMacro( Value, unsigned long, 1 ); }

So that's why BSplineInterpolationWeightFunctionBase::NumberOfWeights became equal to pow(SplineOrder + 1, SpaceDimension), whereas RecursiveBSplineInterpolationWeightFunction::NumberOfWeights just became equal to (SplineOrder + 1) * SpaceDimension! Anyway, it looks like the latter one wasn't really used, so I'm now proposing to just remove the latter one: #1100 https://github.com/SuperElastix/elastix/pull/1100

— Reply to this email directly, view it on GitHub https://github.com/SuperElastix/elastix/pull/1099#issuecomment-2062222060, or unsubscribe https://github.com/notifications/unsubscribe-auth/AG2LMK5VWGOVKIGXNOO6W3TY53MRDAVCNFSM6AAAAABGLIXG6KVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDANRSGIZDEMBWGA . You are receiving this because you were mentioned.Message ID: @.***>

N-Dekker commented 5 months ago