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

BUG: ImageRandomIteratorWithIndex should not assign data in constructor #4183

Conversation

N-Dekker
Copy link
Contributor

@N-Dekker N-Dekker commented Sep 1, 2023

The constructors of ImageRandomConstIteratorWithIndex and ImageRandomConstIteratorWithOnlyIndex that support two arguments (image and region) accidentally still assigned their data members, while they were already initialized by in-class default member initialization. This in-class default member initialization was added as part of pull request #3929 commit 4e46cb6 "STYLE: Default default-constructor of ImageRandom ConstIterator classes", merged on February 24, 2023 and included with tag ITK v5.4rc01.

This caused extra MersenneTwisterRandomVariateGenerator::New() calls, which changed the seeds of random number generation. These changes can possibly cause regression failures in unit tests of client applications, including elastix.

This commit removes all data member assignments from the bodies of these constructors.


The bug was my fault, actually 😧, introduced with commit 4e46cb6

Some background information: Each MersenneTwisterRandomVariateGenerator::New() call changes the random seed. Now it would not be a problem to have the very same MersenneTwisterRandomVariateGenerator::New() call both inside the class definition (in-class default member initialization) and in the constructor member initializer list:

ImageRandomConstIteratorWithIndex<TImage>::ImageRandomConstIteratorWithIndex(const ImageType *  ptr,
                                                                             const RegionType & region)
  : ImageConstIteratorWithIndex<TImage>(ptr, region),
  m_Generator(MersenneTwisterRandomVariateGenerator::New()) // member initializer list
{
}

An initialization in the constructor member initializer list would overrule (supersede) the in-class default member initialization of the same member variable.

However, an assignment to a member variable in the constructor body does not supersede the in-class default member initialization.

{
  // Constructor body. Produces an extra MersenneTwisterRandomVariateGenerator::New() call.
  m_Generator = Statistics::MersenneTwisterRandomVariateGenerator::New();
}

So that's why MersenneTwisterRandomVariateGenerator::New() was called twice, instead of once, when the in-class default member initialization was added.

The constructors of `ImageRandomConstIteratorWithIndex` and
`ImageRandomConstIteratorWithOnlyIndex` that support two arguments (image and
region) accidentally still assigned their data members, while they were already
initialized by in-class default member initialization. This in-class default
member initialization was added as part of pull request
InsightSoftwareConsortium#3929 commit
4e46cb6 "STYLE: Default default-constructor of
ImageRandom ConstIterator classes", merged on February 24, 2023 and included
with tag ITK v5.4rc01.

This caused extra `MersenneTwisterRandomVariateGenerator::New()` calls, which
changed the seeds of random number generation. These changes can possibly cause
regression failures in unit tests of client applications, including elastix.

This commit removes all data member assignments from the bodies of these
constructors.
@github-actions github-actions bot added type:Bug Inconsistencies or issues which will cause an incorrect result under some or all circumstances area:Core Issues affecting the Core module labels Sep 1, 2023
@N-Dekker N-Dekker marked this pull request as ready for review September 1, 2023 23:40
Copy link
Contributor

@tbirdso tbirdso left a comment

Choose a reason for hiding this comment

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

Thanks for tracking this down @N-Dekker . Can you confirm that Elastix regression tests pass again once this fix is applied?

@N-Dekker
Copy link
Contributor Author

N-Dekker commented Sep 5, 2023

Can you confirm that Elastix regression tests pass again once this fix is applied?

Yes I can 😃 Thanks for asking, @tbirdso

Regression failures in elastix + v5.4rc01 branch: https://github.com/SuperElastix/elastix/tree/Upgrade-ITK-to-v5.4rc01

Fixed with this PR branch: https://github.com/SuperElastix/elastix/tree/ImageRandomIteratorWithIndex-should-not-assign-it-constructor

Copy link
Contributor

@tbirdso tbirdso left a comment

Choose a reason for hiding this comment

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

@N-Dekker thanks for confirming this fixes application behavior 👍

@thewtex thewtex merged commit 5032941 into InsightSoftwareConsortium:master Sep 5, 2023
N-Dekker added a commit to N-Dekker/ITKElastix that referenced this pull request Oct 23, 2023
Especially to avoid potential differences between registration results of the
elastix executable and ITKElastix, which were accidentally introduced with ITK
5.4rc1 and fixed with ITK 5.4rc2:

  pull request InsightSoftwareConsortium/ITK#4183
  commit InsightSoftwareConsortium/ITK@997ff54
  BUG: ImageRandomIteratorWithIndex should not assign data in constructor
N-Dekker added a commit to N-Dekker/ITKElastix that referenced this pull request Oct 23, 2023
Especially to avoid potential differences between registration results of the
elastix executable and ITKElastix, which were accidentally introduced with ITK
5.4rc1 and fixed with ITK 5.4rc2:

  pull request InsightSoftwareConsortium/ITK#4183
  commit InsightSoftwareConsortium/ITK@997ff54
  BUG: ImageRandomIteratorWithIndex should not assign data in constructor
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:Core Issues affecting the Core module type:Bug Inconsistencies or issues which will cause an incorrect result under some or all circumstances
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants