stefanklein
commented
4 days ago Hi Niels, I think you are right. This metric should in principle also support the option to run with non-StackTransform. I guess that was never tested with this metric. That code to subtract the mean (for stack and non-stack transform) is actually duplicated in several metrics probably? Maybe a good candidate for refactoring? But then we need some good tests for groupwise registration with different settings. Best, Stefan
From: Niels Dekker @.>
Sent: Monday, June 24, 2024 10:09 PM
To: SuperElastix/elastix @.>
Cc: Stefan Klein @.>; Mention @.>
Subject: Re: [SuperElastix/elastix] BUG: SumOfPairwiseCorrelationCoefficientsMetric TransformIsStack false (PR #1163)
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 Components/Metrics/SumOfPairwiseCorrelationsMetric/itkSumOfPairwiseCorrelationCoefficientsMetric.hhttps://github.com/SuperElastix/elastix/pull/1163#discussion_r1651556385:
- bool m_TransformIsStackTransform{ true };
- bool m_TransformIsStackTransform{ false };
This commit just changes the initial value of m_TransformIsStackTransform, for SumOfPairwiseCorrelationCoefficientsMetric. Without this fix, I think the if (!this->m_TransformIsStackTransform) branch cannot be reached:
@stefankleinhttps://github.com/stefanklein Do you think this fix is OK? Or would it be preferable to just assume that m_TransformIsStackTransform is always true, for this particular metric?
- Reply to this email directly, view it on GitHubhttps://github.com/SuperElastix/elastix/pull/1163#pullrequestreview-2136656483, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AAF2LNOW66T77MB3ZSV3YO3ZJB4D7AVCNFSM6AAAAABJ2OYZEKVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDCMZWGY2TMNBYGM. You are receiving this because you were mentioned.Message ID: @.**@.>>
Initialized
SumOfPairwiseCorrelationCoefficientsMetric::TransformIsStackTransformto false. Otherwise it would always be true.Other metrics (PCAMetric, PCAMetric2, VarianceOverLastDimension) have a similar TransformIsStackTransform property, but those are initialized to false, during the construction of those metrics.