Skip to content

ENH: Improve test coverage of ImageMoments#662

Merged
hjmjohnson merged 6 commits intoInsightSoftwareConsortium:masterfrom
hjmjohnson:improved-image-moments-testing-failed
Apr 1, 2019
Merged

ENH: Improve test coverage of ImageMoments#662
hjmjohnson merged 6 commits intoInsightSoftwareConsortium:masterfrom
hjmjohnson:improved-image-moments-testing-failed

Conversation

@hjmjohnson
Copy link
Copy Markdown
Member

@hjmjohnson hjmjohnson commented Mar 30, 2019

Add masking from SpatialObjects for
computing the ImageMoments.

@hjmjohnson hjmjohnson mentioned this pull request Mar 30, 2019
Closed
dzenanz
dzenanz approved these changes Mar 31, 2019
View reviewed changes
Copy link
Copy Markdown
Member

@dzenanz dzenanz left a comment

Choose a reason for hiding this comment

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

The code looks good to me, but

File: /home/vsts/work/1/s/Modules/Filtering/ImageStatistics/include/itkImageMomentsCalculator.hxx
Line: 150
Description: itk::ERROR: ImageMomentsCalculator(0x1e15d40): Compute(): Total Mass of the image was zero. Aborting here to prevent division by zero later on.

@hjmjohnson
ENH: Improve test coverage of ImageMoments
f7f2462 
Add masking from SpatialObjects for
computing the ImageMoments.
@hjmjohnson
DOC: Expand GetAxisAlignedBoundingBoxRegion replacement code
398b20d 
The template code to replace the often used GetAxisAlignedBoundingBoxRegion
is provided in the documentation to facilitate a suggested
alternate solution.
@hjmjohnson
STYLE: Prefer using Update() for consistency
ec47285 
The ComputeMyBoundingBox and ComputeObjectToWorldTransform
have functionality is moved to protected methods,
ProtectedComputeMyBoundingBox and ProtectedComputeObjectToWorldTransform.

Document preferred method for updating.
Replace use of ComputeMyBoundingBox() with Update();
Replace use of ComputeObjectToWorldTransform() with Update();

Provide legacy methods signatures for ComputeMyBoundingBox and
ComputeObjectToWorldTransform as aliases to Update().
@hjmjohnson hjmjohnson force-pushed the improved-image-moments-testing-failed branch from cba18b8 to 2f3ab79 Compare March 31, 2019 21:32
@hjmjohnson
Copy link
Copy Markdown
Member Author

hjmjohnson commented Mar 31, 2019

@dzenanz The error was the whole point of this patch set. It demonstrates that this code fails where previously it had succeeded (see #663 for the same code passing).

I've been discussing with @aylward and refactored to provide a more clear path to working code. in subsequent patches.

@hjmjohnson hjmjohnson requested a review from aylward March 31, 2019 21:40
@hjmjohnson hjmjohnson self-assigned this Mar 31, 2019
@hjmjohnson hjmjohnson added type:Testing Ensure that the purpose of a class is met/the results on a wide set of test cases are correct type:Style Style changes: no logic impact (indentation, comments, naming) labels Mar 31, 2019
@hjmjohnson
Copy link
Copy Markdown
Member Author

hjmjohnson commented Mar 31, 2019

@aylward it would be great if you could look at these changes.
@N-Dekker This is related to the conversation you and @aylward were having on a different PR. I went ahead and implement part of it because the current implementation was breaking my codebase.

aylward
aylward approved these changes Mar 31, 2019
View reviewed changes
Copy link
Copy Markdown
Member

@aylward aylward left a comment

Choose a reason for hiding this comment

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

The only "bug" seems to be in Modules/Filtering/ImageStatistics/test/itkImageMomentsTest.cxx - where there is still a call to ComputeObjectToWorldTransform() that should probably be a call to Update(). The rest seems perfect. Great work! Darn fast work too!!!

@hjmjohnson hjmjohnson force-pushed the improved-image-moments-testing-failed branch 2 times, most recently from a7d0aa0 to 655bd62 Compare April 1, 2019 13:28
@hjmjohnson
ENH: Avoid deprecated SpatialObject function calls
8d8dea2 
Remove the use of legacy deprecated interfaces from ITK use.

Use Update() instead of ComputeMyBoundingBox().
Use Update() instead of ComputeObjectToWorldTransform().
@hjmjohnson
ENH: ProtectedComputeMyBoundingBox return not used
8a3cdc6 
The ProtectedComputeMyBoundingBox return value is
never used.  The value was not propogated through
the Update call.  In the few casees where false was
returned, an exception is now thrown.
@hjmjohnson hjmjohnson force-pushed the improved-image-moments-testing-failed branch from 655bd62 to 659a1dd Compare April 1, 2019 17:44
@hjmjohnson hjmjohnson mentioned this pull request Apr 1, 2019
Closed
dzenanz
dzenanz approved these changes Apr 1, 2019
View reviewed changes
Copy link
Copy Markdown
Member

@dzenanz dzenanz left a comment

Choose a reason for hiding this comment

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

Looks good, only minor comments

@hjmjohnson
ENH: Clean up tests to pass under new API
bf84697 
The ordering of the API calls is needed
in order to pass the tests.  These tests
now conform to the simplified API for
keeping the state of the spatial object
transforms consistent.
@hjmjohnson hjmjohnson force-pushed the improved-image-moments-testing-failed branch from 659a1dd to bf84697 Compare April 1, 2019 20:48
@hjmjohnson hjmjohnson merged commit fd77f2c into InsightSoftwareConsortium:master Apr 1, 2019
hjmjohnson
hjmjohnson commented Apr 1, 2019
View reviewed changes
Modules/Core/SpatialObjects/test/itkEllipseSpatialObjectTest.cxx
}
std::cout<<"[PASSED]"<<std::endl;

myEllipse->ComputeFamilyBoundingBox( EllipseType::MaximumDepth );
Copy link
Copy Markdown
Member Author

@hjmjohnson hjmjohnson Apr 1, 2019

Choose a reason for hiding this comment

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

@aylward I'm a little concerned that the ordering of these calls changes the behavior. For example, calling "ComputeFamilyBoundingBox" must occur after calling "Update".

Should "ComputeFamilyBoundingBox" implicitly call update?

Copy link
Copy Markdown
Member

@aylward aylward Apr 2, 2019

Choose a reason for hiding this comment

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

As long as ComputeMyBoundingBox() has been called via Update(), there is no need for ComputeFamilyBoundingBox() to call Update(). We perhaps need to make it clear that function calls to get BoundingBoxes, PerimeterSize, Areas, and other values from the objects must occur after defining the parameters of the object and then calling Update(). I think this is a logical flow, and relatively easy to document. My concern with calling Update() from ComputeFamilyBoundingBox() or any other function is that Update() could be a computationally expensive call. We would need to make certain all computations within Update() are conditional on modified times, and they are not at this time. One thing I did not clear up in the refactoring is the odd handling of modified times by certain spatial objects - there are still some inconsistencies (and some pretzel logic, imo), but I think that the appropriate use of Update() mitigates those deficiencies and will not change behavior once they are resolved.

@hjmjohnson hjmjohnson deleted the improved-image-moments-testing-failed branch October 23, 2019 13:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@aylward aylward aylward approved these changes

@dzenanz dzenanz dzenanz approved these changes

Assignees

@hjmjohnson hjmjohnson

Labels

type:Style Style changes: no logic impact (indentation, comments, naming) type:Testing Ensure that the purpose of a class is met/the results on a wide set of test cases are correct

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@hjmjohnson @aylward @dzenanz