Skip to content
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

Merged
merged 3 commits into from
Mar 26, 2018

Conversation

stelfrich
Copy link
Contributor

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

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

@kephale
Copy link

kephale commented Jul 11, 2017

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

@tinevez
Copy link
Contributor

tinevez commented Jul 14, 2017

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
Copy link
Member

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
Copy link
Contributor Author

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
Copy link
Member

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

@stelfrich
Copy link
Contributor Author

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
Copy link
Member

@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
Copy link
Member

Looks good to me too

@hanslovsky hanslovsky merged commit 2b71c6d into imglib:master Mar 26, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants