Add GeomMask recursion test

[tischi]

@ctrueden

Maybe we can merge this or similar test once we figured out what's going on:

https://github.com/tischi/imglib2-roi/blob/geom-mask-recursion/src/test/java/net/imglib2/roi/GeomMaskMultipleORTest.java

[tischi]

@ctrueden Your patch (https://github.com/imglib/imglib2-roi/issues/60) seems to do the job! It runs much faster now and finishes.

RealMin: 0.0
NumOr: 0
Duration [ms]: 277
RealMin: 0.0
NumOr: 1
Duration [ms]: 0
RealMin: 0.0
NumOr: 2
Duration [ms]: 0
RealMin: 0.0
NumOr: 3
Duration [ms]: 0
RealMin: 0.0
NumOr: 4
Duration [ms]: 3
RealMin: 0.0
NumOr: 5
Duration [ms]: 0
RealMin: 0.0
NumOr: 6
Duration [ms]: 1
RealMin: 0.0
NumOr: 7
Duration [ms]: 2
RealMin: 0.0
NumOr: 8
Duration [ms]: 0
RealMin: 0.0
NumOr: 9
Duration [ms]: 1
RealMin: 0.0
NumOr: 10
Duration [ms]: 0
RealMin: 0.0
NumOr: 11
Duration [ms]: 0
RealMin: 0.0
NumOr: 12
Duration [ms]: 0
RealMin: 0.0
NumOr: 13
Duration [ms]: 0
RealMin: 0.0
NumOr: 14
Duration [ms]: 0
RealMin: 0.0
NumOr: 15
Duration [ms]: 1
RealMin: 0.0
NumOr: 16
Duration [ms]: 0
RealMin: 0.0
NumOr: 17
Duration [ms]: 0
RealMin: 0.0
NumOr: 18
Duration [ms]: 0
RealMin: 0.0
NumOr: 19
Duration [ms]: 0

[tischi]

Your patch is part of this PR.

But...there are now two test failing:

at org.junit.Assert.failNotEquals(Assert.java:835)
    at org.junit.Assert.assertEquals(Assert.java:555)
    at org.junit.Assert.assertEquals(Assert.java:685)
    at net.imglib2.roi.geom.OperatorsTest.testXorMovingOperands(OperatorsTest.java:947)

and

at org.junit.Assert.failNotEquals(Assert.java:835)
    at org.junit.Assert.assertEquals(Assert.java:555)
    at org.junit.Assert.assertEquals(Assert.java:685)
    at net.imglib2.roi.geom.OperatorsTest.testOrMovingOperands(OperatorsTest.java:578)

Those tests ran without the patch.

[ctrueden]

Just from the name testOrMovingOperands... do you know whether interval bounds can ever be mutable? My patch assumes that interval bounds are immutable, and therefore cacheable. If they are mutable, then we are sunk, because we cannot cache the result. Or at least: we need to invalidate the cache whenever bounds change, which I'm not sure how we would detect.

Also, even if this works, any PR adding this speedup should also do the same for other classes besides only UnionRealInterval; e.g. I assume the same problem would occur with and, not only or?

Finally, I'm still rather shocked that we have this performance issue in the first place. Would be better to dig into why it's so slow before just slapping a cache on top.

[tischi]

Just from the name testOrMovingOperands... do you know whether interval bounds can ever be mutable?

I think they can be mutable, e.g. I looked for some classes that implement RealInterval and found one that has a method to update the min and max.

Finally, I'm still rather shocked that we have this performance issue in the first place. Would be better to dig into why it's so slow before than just slapping a cache on top.

I agree, to me it feels as if something goes wrong here, i.e. some weird recursion. Is there still some developer around that was involved in designing this part for the code (I guess that would be the most effective to fix this without breaking other things)? If not, I could try digging into it.

[ctrueden]

Is there still some developer around that was involved in designing this part for the code (I guess that would be the most effective to fix this without breaking other things)?

Not really. @awalter17 works at KNIME now. And @tpietzsch, although he collaborated with @awalter17 on this work, is always chock full of priorities to work on. And as usual, I sort of (but not fully) know how all this stuff works, but am drowning in work. So it really would be best if you are able to dig yourself; that's the point of open source.

[tischi]

closing in favor of https://github.com/imglib/imglib2-roi/pull/62