From 9b2b22cc998f39523ab7fd371b88c0beb13d5525 Mon Sep 17 00:00:00 2001 From: Niels Dekker Date: Mon, 27 Mar 2023 17:10:07 +0200 Subject: [PATCH] ENH: Throw exception if "NumberOfResolutions" parameter has value zero Distinguished clearly between zero and one. In this case, the value zero must be a mistake. Improved consistency between fixed and moving pyramid. Discussed at our weekly internal elastix sprint meeting. --- ...MultiMetricMultiResolutionRegistration.hxx | 5 +++ .../elxMultiResolutionRegistration.hxx | 5 +++ ...ultiResolutionRegistrationWithFeatures.hxx | 4 ++ Components/elxGenericPyramidHelper.h | 2 +- .../elxFixedImagePyramidBase.hxx | 2 +- .../elxMovingImagePyramidBase.hxx | 3 +- .../itkElastixRegistrationMethodGTest.cxx | 41 +++++++++++++++++++ 7 files changed, 58 insertions(+), 4 deletions(-) diff --git a/Components/Registrations/MultiMetricMultiResolutionRegistration/elxMultiMetricMultiResolutionRegistration.hxx b/Components/Registrations/MultiMetricMultiResolutionRegistration/elxMultiMetricMultiResolutionRegistration.hxx index fd5c4b35e..19ac1ea1e 100644 --- a/Components/Registrations/MultiMetricMultiResolutionRegistration/elxMultiMetricMultiResolutionRegistration.hxx +++ b/Components/Registrations/MultiMetricMultiResolutionRegistration/elxMultiMetricMultiResolutionRegistration.hxx @@ -49,6 +49,11 @@ MultiMetricMultiResolutionRegistration::BeforeRegistration() /** Set the number of resolutions.*/ unsigned int numberOfResolutions = 3; this->m_Configuration->ReadParameter(numberOfResolutions, "NumberOfResolutions", 0); + if (numberOfResolutions == 0) + { + itkExceptionMacro("The NumberOfResolutions parameter must have a non-zero value!"); + } + this->SetNumberOfLevels(numberOfResolutions); /** Set the FixedImageRegions to the buffered regions. */ diff --git a/Components/Registrations/MultiResolutionRegistration/elxMultiResolutionRegistration.hxx b/Components/Registrations/MultiResolutionRegistration/elxMultiResolutionRegistration.hxx index 626f57c17..09b4fb697 100644 --- a/Components/Registrations/MultiResolutionRegistration/elxMultiResolutionRegistration.hxx +++ b/Components/Registrations/MultiResolutionRegistration/elxMultiResolutionRegistration.hxx @@ -54,6 +54,11 @@ MultiResolutionRegistration::BeforeRegistration() /** Set the number of resolutions. */ unsigned int numberOfResolutions = 3; this->m_Configuration->ReadParameter(numberOfResolutions, "NumberOfResolutions", 0); + if (numberOfResolutions == 0) + { + itkExceptionMacro("The NumberOfResolutions parameter must have a non-zero value!"); + } + this->SetNumberOfLevels(numberOfResolutions); /** Set the FixedImageRegion. */ diff --git a/Components/Registrations/MultiResolutionRegistrationWithFeatures/elxMultiResolutionRegistrationWithFeatures.hxx b/Components/Registrations/MultiResolutionRegistrationWithFeatures/elxMultiResolutionRegistrationWithFeatures.hxx index f848713b6..dfabb54cf 100644 --- a/Components/Registrations/MultiResolutionRegistrationWithFeatures/elxMultiResolutionRegistrationWithFeatures.hxx +++ b/Components/Registrations/MultiResolutionRegistrationWithFeatures/elxMultiResolutionRegistrationWithFeatures.hxx @@ -38,6 +38,10 @@ MultiResolutionRegistrationWithFeatures::BeforeRegistration() /** Set the number of resolutions. */ unsigned int numberOfResolutions = 3; this->m_Configuration->ReadParameter(numberOfResolutions, "NumberOfResolutions", 0); + if (numberOfResolutions == 0) + { + itkExceptionMacro("The NumberOfResolutions parameter must have a non-zero value!"); + } this->SetNumberOfLevels(numberOfResolutions); /** Set the FixedImageRegions to the buffered regions. */ diff --git a/Components/elxGenericPyramidHelper.h b/Components/elxGenericPyramidHelper.h index 2146797a7..f9d7d055e 100644 --- a/Components/elxGenericPyramidHelper.h +++ b/Components/elxGenericPyramidHelper.h @@ -65,7 +65,7 @@ class GenericPyramidHelper configuration.ReadParameter(numberOfResolutions, "NumberOfResolutions", 0, true); if (numberOfResolutions == 0) { - numberOfResolutions = 1; + itkGenericExceptionMacro("The NumberOfResolutions parameter must have a non-zero value!"); } /** Create a default schedule. Set the numberOfLevels first. */ diff --git a/Core/ComponentBaseClasses/elxFixedImagePyramidBase.hxx b/Core/ComponentBaseClasses/elxFixedImagePyramidBase.hxx index 0e954cddd..c82069f7e 100644 --- a/Core/ComponentBaseClasses/elxFixedImagePyramidBase.hxx +++ b/Core/ComponentBaseClasses/elxFixedImagePyramidBase.hxx @@ -103,7 +103,7 @@ FixedImagePyramidBase::SetFixedSchedule() configuration.ReadParameter(numberOfResolutions, "NumberOfResolutions", 0, true); if (numberOfResolutions == 0) { - numberOfResolutions = 1; + itkExceptionMacro("The NumberOfResolutions parameter must have a non-zero value!"); } /** Create a default schedule. Set the numberOfLevels first. */ diff --git a/Core/ComponentBaseClasses/elxMovingImagePyramidBase.hxx b/Core/ComponentBaseClasses/elxMovingImagePyramidBase.hxx index 303a4a9b3..518640b1c 100644 --- a/Core/ComponentBaseClasses/elxMovingImagePyramidBase.hxx +++ b/Core/ComponentBaseClasses/elxMovingImagePyramidBase.hxx @@ -104,9 +104,8 @@ MovingImagePyramidBase::SetMovingSchedule() configuration.ReadParameter(numberOfResolutions, "NumberOfResolutions", 0, true); if (numberOfResolutions == 0) { - log::error("ERROR: NumberOfResolutions not specified!"); + itkExceptionMacro("The NumberOfResolutions parameter must have a non-zero value!"); } - /** \todo quit program? Actually this check should be in the ::BeforeAll() method. */ /** Create a default schedule. Set the numberOfLevels first. */ this->GetAsITKBaseType()->SetNumberOfLevels(numberOfResolutions); diff --git a/Core/Main/GTesting/itkElastixRegistrationMethodGTest.cxx b/Core/Main/GTesting/itkElastixRegistrationMethodGTest.cxx index fa4e51dba..2912fd411 100644 --- a/Core/Main/GTesting/itkElastixRegistrationMethodGTest.cxx +++ b/Core/Main/GTesting/itkElastixRegistrationMethodGTest.cxx @@ -203,6 +203,47 @@ GTEST_TEST(itkElastixRegistrationMethod, IsDefaultInitialized) } +// Tests that the value zero is rejected for the "NumberOfResolutions" parameter. +GTEST_TEST(itkElastixRegistrationMethod, RejectZeroValueForNumberOfResolution) +{ + constexpr auto ImageDimension = 2U; + using PixelType = float; + using ImageType = itk::Image; + const itk::Size imageSize{ { 5, 6 } }; + + elx::DefaultConstruct parameterObject{}; + elx::DefaultConstruct> registration{}; + + registration.SetFixedImage(CreateImage(imageSize)); + registration.SetMovingImage(CreateImage(imageSize)); + registration.SetParameterObject(¶meterObject); + + elx::ParameterObject::ParameterMapType parameterMap{ // Parameters in alphabetic order: + { "ImageSampler", { "Full" } }, + { "MaximumNumberOfIterations", { "2" } }, + { "Metric", { "AdvancedNormalizedCorrelation" } }, + { "Optimizer", { "AdaptiveStochasticGradientDescent" } }, + { "Transform", { "TranslationTransform" } } + }; + + parameterObject.SetParameterMap(parameterMap); + + // OK: "NumberOfResolutions" is unspecified, so use its default value. + EXPECT_NO_THROW(registration.Update()); + + for (const unsigned int numberOfResolutions : { 1, 2 }) + { + // OK: Use a value greater than zero. + parameterMap["NumberOfResolutions"] = { std::to_string(numberOfResolutions) }; + EXPECT_NO_THROW(registration.Update()); + } + + // Expected to be rejected: the value zero. + parameterMap["NumberOfResolutions"] = { "0" }; + EXPECT_THROW(registration.Update(), itk::ExceptionObject); +} + + // Tests registering two small (5x6) binary images, which are translated with respect to each other. GTEST_TEST(itkElastixRegistrationMethod, Translation) {