search

imglib / imglib2-roi

Regions of interest (ROIs) and labelings for ImgLib2
Other
8 stars 8 forks source link

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

Closed tischi closed 1 year ago

tischi commented 2 years ago

Work along the outline provided here: https://github.com/imglib/imglib2-roi/issues/60

tischi commented 2 years ago

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

I implemented:

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

tpietzsch commented 1 year ago

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

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 commented 1 year ago

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

tischi commented 1 year ago

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:

https://github.com/imglib/imglib2-roi/blob/0f2177c818f5d48b934ef2ce8c569dcafef4833a/src/main/java/net/imglib2/roi/Bounds.java#L694

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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

tpietzsch commented 1 year ago

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

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

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

  1. Why is there no return statement after

no return statement after what?

Thanks for digging into it!!! 👍

tischi commented 1 year ago

@tpietzsch

OK, I think I did everything:

  1. https://github.com/imglib/imglib2-roi/pull/63/commits/e4a9054b60995844e4514b47e9d3e6910d769ff0
  2. https://github.com/imglib/imglib2-roi/pull/63/commits/8333f92b4469aba11c022f8e9e01e8101aa08f6b
  3. https://github.com/imglib/imglib2-roi/pull/63/commits/7e9dda103d9361e58602a4489c5a4898d24dcad1

Tests are passing.

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