imglib / imglib2-algorithm

Image processing algorithms for ImgLib2
http://imglib2.net/
Other
22 stars 20 forks source link

getStructuringElementBoundingBox() for Shapes #27

Closed stelfrich closed 6 years ago

stelfrich commented 8 years ago

Adds Shape.getStructuringElementBoundingBox() and default implementations to the subclasses.

This PR addresses the remaining point of a generic bounding box of #5.

kephale commented 7 years ago

@tpietzsch whenever you get a chance, this one is of interest.

tinevez commented 7 years ago

This would be useful to replace this method in the morphology package:  https://github.com/imglib/imglib2-algorithm/blob/master/src/main/java/net/imglib2/algorithm/morphology/MorphologyUtils.java#L97-L132

hanslovsky commented 6 years ago

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.

stelfrich commented 6 years ago

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.

hanslovsky commented 6 years ago

Sounds good to me. Let's keep the special case implementations but provide a reasonable default.

stelfrich commented 6 years ago

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 Shape that behaves like a RectangleShape.

hanslovsky commented 6 years ago

@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.

tpietzsch commented 6 years ago

Looks good to me too