ENH: Exclude retrieval of filenames of masks from elastix library

[N-Dekker]

Avoided confusing log messages saying "-fMask unspecified, so no fixed mask used" and "-mMask unspecified, so no moving mask used" for library users (including ITKElastix).

Relates to issue InsightSoftwareConsortium/ITKElastix#127 "registration with mask does not work", reported by jslalu, June 11, 2021.

---

When reviewing, it may be helpful to ignore whitespace changes: https://github.com/SuperElastix/elastix/pull/895/files?w=1 So this pull request places the GenerateFileNameContainer calls for masks and the related log messages inside the existing if (!BaseComponent::IsElastixLibrary()) ("if not library") clause.

[ntatsisk]

Thanks @N-Dekker, looks good at first glance. However, I have two questions:

• Maybe is it better to remove the prints from ElastixBase altogether? Because I assume everything below the -mMask will still be logged. Right?

30: ELASTIX version: 5.1.0
30: Command line options from ElastixBase:
30: -f D:/a/elastix/elastix/Elastix-source/Testing/Data/3DCT_lung_baseline.mha
30: -m D:/a/elastix/elastix/Elastix-source/Testing/Data/3DCT_lung_followup.mha
30: -fMask unspecified, so no fixed mask used
30: -mMask unspecified, so no moving mask used
30: -out D:\a\elastix\elastix\Elastix-build\Testing\elastix_run_3DCT_lung.NC.euler.ASGD.001
30: -p D:/a/elastix/elastix/Elastix-source/Testing/Data/parameters.3D.NC.euler.ASGD.001.txt
30: -priority unspecified, so NORMAL process priority
30: -threads unspecified, so all available threads are used
30: Command line options from TransformBase:
30: -t0 unspecified, so no initial transform used

• Does it make sense to enable logging in one of the ElastixRegistrationMethod tests so that we can visually check the log more easily? I see that only the command line tests output the log right now.

[N-Dekker]

Thanks @ntatsisk

Maybe is it better to remove the prints from ElastixBase altogether?

Well, the print statement for "-priority" may indeed be removed, as the priority is untouched by the library. However "-out", "-threads", and "-t0" are actually specified by the library, internally, by ElastixRegistrationMethod::GenerateData(). So they might be useful for debugging purposes...

Also I'd rather not add too many "if this is not the library" clauses to the core code. @mstaring @stefanklein What do you think?

[N-Dekker]

@ntatsisk Shall I already merge this one, as a tiny step forward? It doesn't address the question how much logging is useful in general, for library users. But it does at least "mitigate" the problem of issue InsightSoftwareConsortium/ITKElastix#127 "registration with mask does not work".

[ntatsisk]

Sure @N-Dekker, feel free to merge it from my side. Do you mind sharing your intuition also about the following?

• Does it make sense to enable logging in one of the ElastixRegistrationMethod tests so that we can visually check the log more easily? I see that only the command line tests output the log right now.

[N-Dekker]

Does it make sense to enable logging in one of the ElastixRegistrationMethod tests so that we can visually check the log more easily?

Maybe, but in general, I prefer to keep the output of a passing test minimal. In general I think a successfully passing test should just report "passed" to the dashboard. Too much output from a successful test can easily hide relevant information. I know, traditional ITK tests and elastix tests tend to be very verbose, even when they are passing successfully, and I don't really like it.

For a failing unit test, I would like to see more information on the dashboard, though. "Internal elastix error: See elastix log (use LogToConsoleOn() or LogToFileOn())" isn't always enough for me. 🤔

We can make an exception for your case, of course, but let's not make unnecessary noise at the unit test dashboard!

[N-Dekker]

So I just switched on LogToConsole, at e04b09e, and now it says, at https://my.cdash.org/test/81063203

[ RUN      ] itkElastixRegistrationMethod.IsDefaul...
[This part of the test output was removed since it exceeds the threshold of 1024 bytes.]

😢

Apparently that's a CTest feature: https://cmake.org/cmake/help/latest/variable/CTEST_CUSTOM_MAXIMUM_PASSED_TEST_OUTPUT_SIZE.html#variable:CTEST_CUSTOM_MAXIMUM_PASSED_TEST_OUTPUT_SIZE

[ntatsisk]

@N-Dekker, That's confusing to me... how can the rest of the tests provide the complete log output then? They are still run via CTest, right?

[N-Dekker]

how can the rest of the tests provide the complete log output then?

Do they? https://my.cdash.org/test/81070708 says:

elastix is started at Wed May 24 14:44:24 2023.

which elastix:   D:/a/elastix/elastix/Elastix-build/bin/elastix.exe
  elastix version: 5.1.0
  Git revision SHA: 23ef4329e25e0751ebf26ba59d8c85c539f13f80
  Git revision date: Wed May 24 16:03:03 2023 +0200
  Build date: May 24 2023 14:34:56
  Compiler: Visual C++ version 192930148.0
  Memory address size: 64-bit
  CMake version: 3.24.2
  ITK version: 5.3.0

Command-line arguments: 
  -f D:/a/elastix/elastix/Elastix-source/Testing/Data/3DCT_lung_baseline.mha -m D:/a/elastix/elastix/Elastix-source/Testing/Data/3DCT_lung_followup.mha -t0 D:/a/elastix/elastix/Elastix-source/Testing/Data/transformparameters.3DCT_lung.affine.txt -p D:/a/elastix/elastix/Elastix-source/Testing/Data/parameters.3D.MI.bspline.ASGD.001.txt -out D:/a/elastix/elastix/Elastix-build/Testing/elastix_run_3DCT_lung.MI.bspline.ASGD.001

elastix runs at: fv-az439-447
  Windows  DataCenter Server (x64),  (Build 9200)
  with 7167 MB memory, and 2 cores @ 2793 MHz.
-----------------------------------...
[This part of the test output was removed since it exceeds the threshold of 1024 bytes.]

I guess it's a new feature!

[ntatsisk]

OK then it is cdash vs CI difference because the same test gives the entire output in the CI: https://github.com/SuperElastix/elastix/actions/runs/5070534017/jobs/9105697856?pr=898#step:14:10577

[N-Dekker]

Temporarily added a test that produces a lot of output, for you 😺

GTEST_TEST(itkTransformixFilter, GetTransformParameterObjectFromRegistration):

elastix/Core/Main/GTesting/itkTransformixFilterGTest.cxx

Lines 1465 to 1490 in 99947da

	GTEST_TEST(itkTransformixFilter, GetTransformParameterObjectFromRegistration)
	{
	constexpr auto imageDimension = 2U;
	using PixelType = float;
	using ImageType = itk::Image<PixelType, imageDimension>;
	// Use minimum size images, to make the test run a little bit faster.
	const auto imageSize = itk::Size<imageDimension>::Filled(minimumImageSizeValue);
	// In order to keep the test simple, just fill both the fixed and moving image with 1, 2, 3, ...
	const auto fixedImage = CreateImageFilledWithSequenceOfNaturalNumbers<PixelType>(imageSize);
	const auto movingImage = CreateImageFilledWithSequenceOfNaturalNumbers<PixelType>(imageSize);
	elx::DefaultConstruct<itk::ElastixRegistrationMethod<ImageType, ImageType>> registration{};
	elx::DefaultConstruct<itk::TransformixFilter<ImageType>> transformixFilter{};
	// Do a registration, using the default parameter maps of ElastixRegistrationMethod. May take some time!
	registration.LogToConsoleOn();
	registration.SetFixedImage(fixedImage);
	registration.SetMovingImage(movingImage);
	registration.Update();
	transformixFilter.LogToConsoleOn();
	transformixFilter.SetMovingImage(movingImage);
	transformixFilter.SetTransformParameterObject(registration.GetTransformParameterObject());
	transformixFilter.Update();

Output at https://github.com/SuperElastix/elastix/actions/runs/5072498636/jobs/9110282589

From here: https://github.com/SuperElastix/elastix/actions/runs/5072498636/jobs/9110282589#step:14:28981

As well as here: https://dev.azure.com/kaspermarstal/Elastix/_build/results?buildId=4121&view=logs&j=34a65711-f108-577e-6707-abb292f048dd&t=97b2d3e0-53b7-57b8-cbbf-64a4a6e9d9bd&l=28559