ENH: Add support for multiple metrics and regularization#1437
ENH: Add support for multiple metrics and regularization#1437Leirbag-gabrieL wants to merge 1 commit into
Conversation
|
Thank you very much @Leirbag-gabrieL Can you possibly make a minimal example of a registration that is made possible by your proposed fix? Ideally in the form of a library unit test, like this one: elastix/Core/Main/GTesting/itkElastixRegistrationMethodGTest.cxx Lines 3006 to 3048 in 0c0b19f But then, having something like Otherwise, a complete ("dummy") test set, including elastix parameter file, would also be of help to us. Again, preferably "minimal", so that it won't take much time to process. |
| using MovingImageDerivativeScalesType = FixedArray<double, Self::MovingImageDimension>; | ||
|
|
||
| /** Typedef for transform penalty metrics. */ | ||
| typedef TransformPenaltyTerm<TFixedImage> TransformMetricType; |
There was a problem hiding this comment.
Nowadays, we are using using, instead of typedef. But a coding style issue like that is not a show-stopper. I can adjust it afterwards. Also, I think I would move the definition of TransformMetricType into the function body of CombinationImageToImageMetric::SetMetric, as that seems to be the only place where it is needed.
|
|
||
| /** Typedef for transform penalty metrics. */ | ||
| typedef TransformPenaltyTerm<TFixedImage> TransformMetricType; | ||
| typedef typename TransformMetricType::Pointer TransformMetricPointer; |
There was a problem hiding this comment.
TransformMetricPointer is never used, so I think I would remove it. (No show-stopper for now.)
| TransformMetricType * testPtr2 = dynamic_cast<TransformMetricType *>(metric); | ||
|
|
||
| // Increase newly defined numberOfMetric counters | ||
| if (testPtr1) |
There was a problem hiding this comment.
I would make it:
if( dynamic_cast<PointSetMetricType *>(metric) )
And remove the local testPtr1 variable.
Similar for testPtr2, oldTestPtr1, oldTestPtr2. No show-stopper to me, though. Just a style thing.
|
| if ((this->GetNumberOfMovingImages() != 1) && (this->GetNumberOfMovingImages() != nrOfMetrics)) | ||
| if ((this->GetNumberOfMovingImages() != 1) && (this->GetNumberOfMovingImages() != nrOfImageMetrics)) | ||
| { | ||
| itkExceptionMacro("The NumberOfMovingImages should equal 1 or equal the NumberOfMetrics"); | ||
| itkExceptionMacro("The NumberOfMovingImages should equal 1 or equal the nrOfImageMetrics"); |
There was a problem hiding this comment.
@stefanklein @mstaring Would this change (comparing the number of input images with the number of image metrics, instead of the total number of metrics) break legacy use cases that would use "dummy" images when they use multiple metrics?
If so, shall we still allow such legacy use cases, by relaxing this check?
|
Hi Niels,
I think it would indeed be a nice backward compatibility approach, if we still could allow the case when NumberOfMovingImages equals the NumberOfMetrics.
Best, Stefan
Van: Niels Dekker ***@***.***>
Verzonden: donderdag 18 juni 2026 15:36
Aan: SuperElastix/elastix ***@***.***>
CC: Stefan Klein ***@***.***>; Mention ***@***.***>
Onderwerp: Re: [SuperElastix/elastix] ENH: Add support for multiple metrics and regularization (PR #1437)
@N-Dekker commented on this pull request.
________________________________
In Components/Registrations/MultiMetricMultiResolutionRegistration/itkMultiMetricMultiResolutionImageRegistrationMethod.hxx<#1437 (comment)>:
- if ((this->GetNumberOfMovingImages() != 1) && (this->GetNumberOfMovingImages() != nrOfMetrics))
+ if ((this->GetNumberOfMovingImages() != 1) && (this->GetNumberOfMovingImages() != nrOfImageMetrics))
{
- itkExceptionMacro("The NumberOfMovingImages should equal 1 or equal the NumberOfMetrics");
+ itkExceptionMacro("The NumberOfMovingImages should equal 1 or equal the nrOfImageMetrics");
@stefanklein<https://github.com/stefanklein> @mstaring<https://github.com/mstaring> Would this change (comparing the number of input images with the number of image metrics, instead of the total number of metrics) break legacy use cases that would use "dummy" images when they use multiple metrics?
If so, shall we still allow such legacy use cases, by relaxing this check?
-
Reply to this email directly, view it on GitHub<#1437?email_source=notifications&email_token=AAF2LNKV7IUHUQI57BH7UET5APV3LA5CNFSNUABKM5UWIORPF5TWS5BNNB2WEL2QOVWGYUTFOF2WK43UKJSXM2LFO4XTINJSGUZTKNJYGYZKM4TFMFZW63VHNVSW45DJN5XKKZLWMVXHJLDGN5XXIZLSL5RWY2LDNM#pullrequestreview-4525355862>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AAF2LNN2T5GSIBEIEY54IM35APV3LAVCNFSNUABEKJSXA33TNF2G64TZHM4TCNJYGY4TINB3JFZXG5LFHM2DGNBRGI4DGNZTGWQXMAQ>.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS<https://github.com/notifications/mobile/ios/AAF2LNNILPEGO2JK2DTSZJ35APV3LA5CNFSNUABKM5UWIORPF5TWS5BNNB2WEL2QOVWGYUTFOF2WK43UKJSXM2LFO4XTINJSGUZTKNJYGYZKM4TFMFZW63VHNVSW45DJN5XKKZLWMVXHJKTGN5XXIZLSL5UW64Y> and Android<https://github.com/notifications/mobile/android/AAF2LNJ4DPPQXUV7Q7P5GF35APV3LA5CNFSNUABKM5UWIORPF5TWS5BNNB2WEL2QOVWGYUTFOF2WK43UKJSXM2LFO4XTINJSGUZTKNJYGYZKM4TFMFZW63VHNVSW45DJN5XKKZLWMVXHJLTGN5XXIZLSL5QW4ZDSN5UWI>. Download it today!
You are receiving this because you were mentioned.Message ID: ***@***.******@***.***>>
|
|
Hi, sorry for responding so late, to answer your question @N-Dekker yes I totally took @alvarez-pa 's pull request as a strating point to do mine. I think I should not do it since you just did this one #1453 right ? |
|
Thanks for your work, @Leirbag-gabrieL My intention is indeed to merge PR #1453, and then close both PRs of you and @alvarez-pa. But your commit is still there in PR #1453, so your name will remain in our git log history 😃 |
Hi, apparently it was not possible to do a multi-image registration with a transform metric like this:
Now it is possible :)