-
Notifications
You must be signed in to change notification settings - Fork 20
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
getStructuringElementBoundingBox() for Shapes #27
Conversation
@tpietzsch whenever you get a chance, this one is of interest. |
This would be useful to replace this method in the morphology package: |
It would be great to know the bounding boxes of shapes (centered at zero). Thank you for approaching this @stelfrich Having a meaningful default implementation would reduce the number of lines at the cost of performance: public default Interval getStructuringElementBoundingBox(final int numDimensions) {
final RandomAccessible< Object > accessible = ConstantUtils.constantRandomAccessible( null, numDimensions );
final RandomAccess< Neighborhood< Object > > access = neighborhoodsRandomAccessible( accessible ).randomAccess();
access.setPosition( new long[ numDimensions ] );
final long[] min = LongStream.generate( () -> Long.MAX_VALUE ).limit( numDimensions ).toArray();
final long[] max = LongStream.generate( () -> Long.MIN_VALUE ).limit( numDimensions ).toArray();
for ( final Cursor< Object > cursor = access.get().localizingCursor(); cursor.hasNext(); )
{
cursor.fwd();
for ( int d = 0; d < numDimensions; ++d )
{
final long pos = cursor.getLongPosition( d );
min[ d ] = Math.min( pos, min[ d ] );
max[ d ] = Math.max( pos, max[ d ] );
}
}
return new FinalInterval( min, max );
} As this is not a performance critical method (should not be called in tight loops) but rather a convenience, the performance hit would be negligible and we would benefit from a smaller code base. The tests should remain the same as they are in this PR. @stelfrich Note that this is not a prompt to adapt your code right away but rather an open discussion. Opinions from @imglib/developers very much appreciated. |
Thanks for taking time to look at this @hanslovsky! Would it make sense to have the default implementation that you are proposing in addition to the more specific implementations already available? Adding the methods breaks public API anyway, so we don't necessarily have to care about bytecode compatibility. I am, however, also fine with getting rid of the more specific implementations if we point out in the documentation that calling this method in a loop might be a bad idea. |
Sounds good to me. Let's keep the special case implementations but provide a reasonable default. |
I have added your default implementation, @hanslovsky, and a small unit test with an anonymous inner |
@imglib/admins This looks good to me. Even though it is an interface breaking change it should not break any code downstream with the reasonable default implementation. Please confirm before I merge. |
Looks good to me too |
Adds
Shape.getStructuringElementBoundingBox()
and default implementations to the subclasses.This PR addresses the remaining point of a generic bounding box of #5.