search

imglib / imglib2-roi

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

Speed up DefaultWritableRealPointCollection #49

Closed kephale closed 5 years ago

kephale commented 5 years ago

Break loop early during test method when possible

ctrueden commented 5 years ago

Thanks @kephale. But we could make it even better, no? We don't actually need to compute any distances. It would suffice to return true if Util.locationsEqual(pt, l), and false otherwise. Right?

Since you optimized this—I assume you have a performance-sensitive application handy. Could you test the equality idea to see how much more that improves things?

If you are feeling ambitious, you could also write your own version of that method with the dimensionality test outside the loop instead of inside, and see if that affects anything.

kephale commented 5 years ago

Yes and no, there actually seems to be an error upstream in imglib2. The equality checks on doublePositions are vulnerable to precision errors. I'll poke a little but if it takes too long I'll probably just leave this dangling.

Upstream error is here: https://github.com/imglib/imglib2/blob/e7d907cb0572c26be87bb91020981adbf248cf45/src/main/java/net/imglib2/util/Util.java#L977

kephale commented 5 years ago

So actually the answer is no.

https://github.com/imglib/imglib2-roi/blob/c6925c158b51a49558a12d7183c6b1a06d6f1981/src/main/java/net/imglib2/roi/geom/real/DefaultWritablePointMask.java#L84 requires that the input is a PointMask, but this test is against a RealLocalizable.

ctrueden commented 5 years ago

requires that the input is a PointMask, but this test is against a RealLocalizable.

That's why I suggested using Util.locationsEqual(pt, l) instead of pt.equals(l). Equality is defined more strictly for ImgLib2 ROIs—both the content and the type need to match. The equals javadoc explains it, IIRC. But you can use Util.locationsEqual to compare only the locations.

The equality checks on doublePositions are vulnerable to precision errors.

We could add Util.locationsEqual(l1, l2, epsilon) signatures if that would help. But I don't know that there is any particular epsilon for which we want to say a point tests positive for the RealPointCollection containment. Right now, there is no "point collection with positive radius around each point" feature, nor do I necessarily think there should be.

Or do you mean some other precision-related problem? Like in the other direction: two points that have the same double-precision position, but are of a type that is more precise than double, and so the Util.locationsEqual returns true erroneously?

kephale commented 5 years ago

My initial guess was incorrect. It is not a precision/numerical comparison issue. The comparison methods you suggest are indeed used, but the issue is actually a typing issue.

The PR that I filed does improve things. To swap to your suggested change some additional methods, including the ones I mentioned have to be altered, I'm out of time to work on this for near future. Merge or delay as you see fit.

ctrueden commented 5 years ago

It is not a precision/numerical comparison issue. The comparison methods you suggest are indeed used, but the issue is actually a typing issue. ... To swap to your suggested change some additional methods, including the ones I mentioned have to be altered

Sorry, but I don't follow your reasoning. Which methods? Where is the typing issue? More detail would be helpful.

I merged as-is, since as you say it is a step forward. Just seems like a missed opportunity to make things better while our attention was focused here.