Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ENH: Refactor itk::ImageRegistrationMethodv4 statements into methods #3876

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

jhlegarreta
Copy link
Member

  • ENH: Refactor itk::ImageRegistrationMethodv4 domain clearing methods
  • ENH: Group setting itk::ImageRegistrationMethodv4 random seeds
  • ENH: Refactor itk::itkImageRegistrationMethodv4 dependent statements

PR Checklist

@github-actions github-actions bot added area:Registration Issues affecting the Registration module type:Enhancement Improvement of existing methods or implementation labels Jan 19, 2023
Refactor `itk::ImageRegistrationv4` soure and target domain image, mask
and point set ivar clearance statements into methods to improve
encapsulation.
Group setting `itk::ImageRegistrationMethodv4` random seeds into a
method to improve encapsulation.
Refactor `itk::itkImageRegistrationMethodv4` initialization statements
dependent on the number of levels into a method to improve
encapsulation.
@jhlegarreta jhlegarreta force-pushed the RefactorRegistrationMethodv4StatementsIntoMethods branch from ef55328 to 0500c27 Compare January 27, 2023 00:19
@jhlegarreta
Copy link
Member Author

/azp run ITK.macOS

* Initialize the shrink factors, smoothing sigmas, and metric sampling percentage values. If
* decreasingConsecutiveShrinkFactors is true, the shrink factors will be initialized to decreasing integer values
* starting from the number of levels minus one (e.g. if the number of level is $3$, they will be initialized to the
* set ${2,1,0}$; if false, they will be initialized to all $1$'s. An equivalent logic applies to the smoothing sigma
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needs to match whatever decision is made on the constructor/refactoring.

this->m_SmoothingSigmasPerLevel.SetSize(this->m_NumberOfLevels);
this->m_SmoothingSigmasPerLevel[0] = 2;
this->m_SmoothingSigmasPerLevel[1] = 1;
this->m_SmoothingSigmasPerLevel[2] = 0;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @ntustison I would be grateful if you could review this PR as time permits, especially he changes in commit 0500c27. The SetNumberOfLevels call in https://github.com/InsightSoftwareConsortium/ITK/pull/3876/files#diff-63900e690554f3dd0bd22b3205f700be44c592699ca21a4a48b5f184ab8e61cbL103 initializes the above values to some given values https://github.com/InsightSoftwareConsortium/ITK/pull/3876/files#diff-63900e690554f3dd0bd22b3205f700be44c592699ca21a4a48b5f184ab8e61cbR890, but the lines above override such values (I have not removed the existing body yet, but eventually the Initialize* calls should remain only, once the latter as well get polished following your comments). I have not looked into its details, but I am wondering whether that was intended or not, and why, or which would be your recommendations to initialize the values.

Also, I'm wondering whether a rule could be designed for the case where the number of levels is not 3.

Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:Registration Issues affecting the Registration module type:Enhancement Improvement of existing methods or implementation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant