ENH: Improve coverage for itk::FastMarchingImageFilterBase.

[hjmjohnson]

Exercise basic object methods.

Test all Set/Get methods using the TEST_SET_GET_VALUE macro, and all
On/Off methods corresponding to boolean members usign the
TEST_SET_GET_BOOLEAN macro.

Use the TRY_EXPECT_NO_EXCEPTION macro to update the filter and save
tryping the try/catch blocks.

Prefer initialization over assignment in variables.

Use the itkNotUsed macro for test input arguments and hence avoid
casting argc and argv to void to avoid compiler warnings.

Remove printing the input (speed) image.

Remove the calls to the Get* methods corresponding to base class
members (specifically: m_SpeedConstant, m_TargetReachedValue, and
m_NormalizationFactor should be tested for itk::FastMarchingBase in
itkFastMarchingBaseTest.cxx; and m_StoppingValue and GetCollectPoints
should be tested for itk::FastMarchingImageFilter in
itkFastMarchingTest.cxx).

Start comments with capital letters.

Minor style changes (such as white spaces between operators and
arguments, improvement of type names and error messages).

[hjmjohnson]

@jhlegarreta This was almost 2 years old. I hope I rebased properly.

[hjmjohnson]

Jon Hait... Gorroño
Uploaded patch set 1.Apr 25, 2017

Jon Haitz Legarreta Gorroño
Apr 25, 2017
↩
Patch Set 1:

(3 comments)

Modules/Filtering/FastMarching/test/itkFastMarchingImageFilterBaseTest.cxx
Line 72:
I guess we could get away if we removed this. I fear that an input image of all zeros is not the most appropriate to test this class.

Modules/Filtering/FastMarching/test/itkFastMarchingImageFilterRealTest1.cxx
Line 172:
Anybodody knows why "1.42"?

Looks like expected value is about/almost zero:
itk::Math::abs( outputValue ) > itk::NumericTraits< double >::epsilon() * 1.42

Modules/Filtering/FastMarching/test/itkFastMarchingImageFilterRealTest2.cxx
Line 190:
Again, same comment as for itkFastMarchingImageFilterRealTest1.cxx

Kitware Build Robot
Apr 25, 2017
↩
Patch Set 1: Verified-1

Build Failed: CDash filtered results: https://open.cdash.org/index.php?&project=Insight&filtercount=3&field1=buildname/string&compare1=63&value1=22311-1&field2=buildstarttime/date&compare2=83&value2=2015-03-01&field3=buildstarttime/date&compare3=84&value3=2029-01-01

Jon Haitz Legarreta Gorroño
Apr 25, 2017
↩
Patch Set 1:

On the other hand, I'm well aware that setting the output image properties in itkFastMarchingImageFilterBase.cxx is not really meaningful since no real computation/check is performed. Setting them in *RealTest1.cxx or *RealTest2.cxx seemed not be very cautious to me given that my understanding of such implications is limited/was confused about the funny way (the "1.42") the output values were being checked.

Otherwise, we could write the output image in itkFastMarchingImageFilterBase.cxx and check the MD5 hash, using and w/o using the output image properties (i.e. two tests). That may require some a deeper understanding concerning the trial points, etc. which I may not have.

An alternative would be setting such properties in *RealTest1.cxx or *RealTest2.cxx, and checking that the test still passes. If it passes, it would be a little bit hard to understand why such a change does not affect the result/may indicate a bug (?); if not, I ignore how hard it'd be adapting it to check the right values.

Matt McCormick
Apr 27, 2017
↩
Patch Set 1:

(3 comments)

Modules/Filtering/FastMarching/test/itkFastMarchingImageFilterBaseTest.cxx
Line 53:
Non-identity (default) spacing is nice to test.

Line 63:
A non-null origin (the default) is nice to check.

Line 72:
If we set some TrialPoints, run the filter, and get the output, it may be not the most interesting, but we could have something to check?

[hjmjohnson]

Accidently deleted/closed branch when the script for moving from Gerrit went haywire.

[hjmjohnson]

@thewtex @jhlegarreta This issue is almost 2 years old. I spent some time today making this PR pass regression tests. While I am certain that more work could be done, I would like to move this incremental better solution forward to remove it from the 2-year-old backlog. I don't think there is much interest in pursuing this more aggressively at the moment.

[jhlegarreta]

@hjmjohnson thanks for caring about this. I'll have a look at the comments/what was left undone it during the next few days, do the appropriate changes if any, and give some feedback.

[jhlegarreta]

Rebased on master for now. Revision to come later next week.

[hjmjohnson]

@jhlegarreta Can this be pushed as is, and then your additions added once you find the time? I'd hate for this 2-year-old PR to become stale again.

[jhlegarreta]

@hjmjohnson @thewtex Thanks for your patience. I went through the comments. It does not look complicated what is left to improve the robustness of the tests, but it requires some time. As Hans told me a few weeks ago, "Don't let perfection be the enemy of the good", so for the time being, I think it is reasonable to merge it as it is, given that checks are passing, and it is already an improvement and a step towards the right direction.

I've opened an issue (#599) to keep track of the pending tasks. As said, affordable to anyone in the community, useful for all.

Sorry for taking so long to review it.

[hjmjohnson]

@thewtex Now we just need someone to approve, and it can be merged! Let's try to get this in before 2 years have passed 👍 .

[thewtex]

Thanks for the contribution and persistence!