imglib / imglib2-roi

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

Fix ImgLabeling for various types of index images #37

Closed maarzt closed 3 years ago

maarzt commented 6 years ago

The accessors for ImgLabeling are build with the assumption that RandomAccess.get() and Cursor.get() of the index image will always return the same object. This constraint holds for ArrayImg and CellImg, but fails for other RandomAccessibleIntervals like:

This commit removes the assumption, and therefor fixes ImgLabeling for the previously mentioned RandomAccessibleIntervals.

tpietzsch commented 6 years ago

Can you please add benchmarks to show the impact this has on ArrayImg backed ImgLabeling? I clearly see the need to make it work for all RandomAccessibleIntervals, but it would be good to know the performance impact...

maarzt commented 6 years ago

The current code must be considered broken. I wouldn't recommend benchmarking broken code against working code. I consider code correctness the first step, performance the second. But here are the results:

Current implementation:

Benchmark                                    Mode  Cnt      Score      Error  Units
ImgLabelingBenchmark.benchmarkCursor        thrpt    8  34039.611 ± 6137.750  ops/s
ImgLabelingBenchmark.benchmarkRandomAccess  thrpt    8  14425.188 ± 1376.425  ops/s

This PR:

Benchmark                                    Mode  Cnt      Score      Error  Units
ImgLabelingBenchmark.benchmarkCursor        thrpt    8  28140.803 ± 6749.818  ops/s
ImgLabelingBenchmark.benchmarkRandomAccess  thrpt    8  13572.857 ±  767.816  ops/s

10% performance loss, in the specific scientific test cases. The performance loss in real life is probably negligible.

maarzt commented 6 years ago

I changed the benchmark a tiny bit, to now use a larger image, and therefor do a more precise measurement. Conclusion:

ctrueden commented 4 years ago

Wow, this has been open for quite a long time. It had conflicts, so I rebased it over the latest master and force-pushed.

imagejan commented 3 years ago

What's the status of this one? Can it be rebased and merged?

maarzt commented 3 years ago

I think it can be merged. @tpietzsch What do you think? The performance tests are done. 10% slower for ArrayImg cursor.