Implement realMinMax() and minMax() methods to fix adapting interval performances

[tischi]

Work along the outline provided here: #60

[tischi]

@tpietzsch
Could you have a look?
I think I did everything....

I implemented:

• realMinMax for AbstractAdaptingRealInterval
• minMax for AbstractAdaptingInterval

and changed all sub-classes that they only implement this method, while all the other work is done within AbstractAdaptingRealInterval and AbstractAdaptingInterval.

[tpietzsch]

I added a commit where I let AbstractAdaptingInterval extends AbstractAdaptingRealInterval, and implemented realMinMax in terms of minMax.
This will

• make the instanceof checks in getMinMax more robust, in case a AbstractAdaptingInterval is passed into the getMinMax(RealInterval, double[], double[]) version.
• provide efficient implementations of the RealInterval methods.

I added stubs for all the Interval methods that should be overridden. Can you please add the implementations?

RealTransformRealInterval should be revised, relying more on the realMinMax method directly. As is is, that will be called many times., through the various realMin/Max methods. Could you have a look at that as well? I added a few TODO comments for getting started.

[tpietzsch]

Also AbstractAdaptingInterval should override dimensions(Positionable).
I forgot that one...

[tischi]

Thanks @tpietzsch for having a look! I am truly impressed that you spotted

in case a AbstractAdaptingInterval is passed into the getMinMax(RealInterval, double[], double[]) version

you have to explain me at some point how you find such stuff!

---

I can do all the requested changes, however not to screw anything up I would like to understand this part of the code:

imglib2-roi/src/main/java/net/imglib2/roi/Bounds.java

Line 694 in 0f2177c

while ( !Arrays.equals( sMx, cachedSourceMax ) ||

Frankly, I don't see how anything that happens in the while loop would affect the loop's conditional.
I mean, if source does not change (and I don't see how it would), the value of the variables that are tested in the while condition should not change?!

In other words, could this be rewritten without a while loop? Probably I am overlooking something?!

[tpietzsch]

I think the idea is that the source might be changed by another thread. But there is so many more problems if that actually happens... I think it is better to assume that this will be used single-threaded.

So yes, I think it should be rewritten without the while loop.

[tischi]

OK, I will rewrite it then. There are several things though that I don't get...

1. Isn't transformToSource.numTargetDimensions() == source.numDimensions()?
2. There are repeated calls to this.transformToSource.numTargetDimensions().
   Can this change? If not, why not just have a field numSourceDimensions = transformToSource.numTargetDimensions() initialised in the constructor? I feel that if the dimensions of transformToSource.numTargetDimensions() could change, then it maybe transformToSource.numSourceDimensions() could change and then also the initialization of the dimensions in the constructor super(...) would be wrong?
3. If we would do source.realMin( cachedSourceMin ); source.realMax( cachedSourceMax ); at the very beginning of updateMinMax() then we could reuse cachedSourceMin and cachedSourceMax later?!
4. Why is there no return statement after

[tischi]

(at least, i would only change source.numDimensions() once during the updateMinMax()...)

[tpietzsch]

First of all, this isn't originally my code, and it's been a few years since I reviewed it... So I'm also guessing a bit.

1. Isn't transformToSource.numTargetDimensions() == source.numDimensions()?

Yes, let's assume that!

(In principle, I think that you might want to be able to have a 3D source and apply a 2D transformation that leaves the 3rd dimension untouched. But that wouldn't work with the current code. And I think it would make things a lot more complicated for marginal benefit).

2. There are repeated calls to this.transformToSource.numTargetDimensions().
   Can this change? If not, why not just have a field numSourceDimensions = transformToSource.numTargetDimensions() initialised in the constructor? I feel that if the dimensions of transformToSource.numTargetDimensions() could change, then it maybe transformToSource.numSourceDimensions() could change and then also the initialization of the dimensions in the constructor super(...) would be wrong?

Neither transformToSource.numTargetDimensions() nor numSourceDimensions() should ever change.
So yes, it's a good idea to make a field numSourceDimensions = transformToSource.numTargetDimensions() initialised in the constructor.

3. If we would do source.realMin( cachedSourceMin ); source.realMax( cachedSourceMax ); at the very beginning of updateMinMax() then we could reuse cachedSourceMin and cachedSourceMax later?!

Yes, sounds good.

4. Why is there no return statement after

no return statement after what?

Thanks for digging into it!!! 👍

[tischi]

@tpietzsch
I don't understand the code here. If the code is good, shouldn't we put it as a static method somewhere? Something like RealInterval ... = TransformHelper.estimateBounds( RealTransform ..., RealInterval ... );

[bogovicj]

shouldn't we put it as a static method somewhere

I'd like that, as well as an implementation that would work for non-linear transformations too
imglib/imglib2-realtransform#37

[tpietzsch]

I refactored this a bit, and it should be clearer now what happens.

This method computes the corners of the source interval (see javadoc for explanation how)

imglib2-roi/src/main/java/net/imglib2/roi/Bounds.java

Lines 873 to 892 in 46e8890

	/**
	* Compute the corners of a RealInterval given by {@code min} and {@code max}.
	* <p>
	* A n-dimensional interval has {@code 2^n} corners.
	* <p>
	* The index of a corner is interpreted as a binary number where bit 0
	* corresponds to X, bit 1 corresponds to Y, etc. A zero bit means min in the
	* corresponding dimension, a one bit means max in the corresponding dimension.
	*/
	private static double[][] corners( final double[] min, final double[] max )
	{
	assert min.length == max.length;
	final int n = min.length;
	final int numCorners = 1 << n;
	final double[][] corners = new double[ numCorners ][ n ];
	for ( int index = 0; index < numCorners; index++ )
	for ( int d = 0, mask = numCorners >> 1; d < n; ++d, mask >>= 1 )
	corners[ index ][ d ] = ( index & mask ) == 0 ? min[ d ] : max[ d ];
	return corners;
	}

Then this method takes the corners and puts them through the transformation

imglib2-roi/src/main/java/net/imglib2/roi/Bounds.java

Lines 858 to 865 in 46e8890

	private double[][] createCorners()
	{
	final double[][] cornersTransformed = corners( cachedSourceMin, cachedSourceMax );
	final double[][] points = new double[ cornersTransformed.length ][ n ];
	for ( int i = 0; i < points.length; i++ )
	transformToSource.inverse().apply( cornersTransformed[ i ], points[ i ] );
	return points;
	}

Finally, this method puts computes the bounding box of the transformed corners

imglib2-roi/src/main/java/net/imglib2/roi/Bounds.java

Lines 841 to 855 in 46e8890

	final double[][] transformedCorners = createCorners();
	final int numTransformedCorners = transformedCorners.length;
	for ( int d = 0; d < n; d++ )
	{
	double maxCorner = transformedCorners[ 0 ][ d ];
	double minCorner = transformedCorners[ 0 ][ d ];
	for ( int i = 1; i < numTransformedCorners; i++ )
	{
	minCorner = Math.min( minCorner, transformedCorners[ i ][ d ] );
	maxCorner = Math.max( maxCorner, transformedCorners[ i ][ d ] );
	}
	min[ d ] = minCorner;
	max[ d ] = maxCorner;
	}

[tischi]

@tpietzsch

OK, I think I did everything:

1. e4a9054
2. 8333f92
3. 7e9dda1

Tests are passing.

I have one question, which I added as a comment to the last commit.