search

imagej / imagej-ops

ImageJ Ops: "Write once, run anywhere" image processing
https://imagej.net/libs/imagej-ops
BSD 2-Clause "Simplified" License
88 stars 42 forks source link

Optimise BoxCount further #618

Closed rimadoma closed 4 years ago

rimadoma commented 4 years ago

Further optimises ops related to issue https://github.com/bonej-org/BoneJ2/issues/196

Changes execution times for a 256 x 256 x 256 binary image:

rimadoma commented 4 years ago

@gselzer @ctrueden This PR can wait. I'll let you know when "my people" are happy with the performance. Apologies for not mentioning this earlier, and creating extra work.

mdoube commented 4 years ago

when "my people" are happy with the performance

Looks like a major improvement. "This person" is happy with it.

rimadoma commented 4 years ago

OK, this PR is go then. @ctrueden It'd be rad if you had the time to cut a new release of imagej-ops for pom-scijava:29.x.x but no worries.

ctrueden commented 4 years ago

I tried to release a new imagej-ops, but am seeing three new test failures on a local Linux box:

-------------------------------------------------------------------------------
Test set: net.imagej.ops.morphology.outline.OutlineTest
-------------------------------------------------------------------------------
Tests run: 8, Failures: 3, Errors: 0, Skipped: 0, Time elapsed: 0.34 s <<< FAILURE! - in net.imagej.ops.morphology.outline.OutlineTest
testEdgeSquareExcludeEdgesFalse(net.imagej.ops.morphology.outline.OutlineTest)  Time elapsed: 0.046 s  <<< FAILURE!
java.lang.AssertionError: Wrong number of foreground elements in interval expected:<8> but was:<6>
        at net.imagej.ops.morphology.outline.OutlineTest.testEdgeSquareExcludeEdgesFalse(OutlineTest.java:206)

testOutlineSquare(net.imagej.ops.morphology.outline.OutlineTest)  Time elapsed: 0.039 s  <<< FAILURE!
java.lang.AssertionError: Wrong number of foreground elements in interval expected:<8> but was:<5>
        at net.imagej.ops.morphology.outline.OutlineTest.testOutlineSquare(OutlineTest.java:148)

testEdgeSquare(net.imagej.ops.morphology.outline.OutlineTest)  Time elapsed: 0.042 s  <<< FAILURE!
java.lang.AssertionError: Wrong number of foreground elements in interval expected:<7> but was:<5>
        at net.imagej.ops.morphology.outline.OutlineTest.testEdgeSquare(OutlineTest.java:177)

I could not reproduce on macOS, and Travis CI also didn't seem to have a problem.

@rimadoma Any thoughts?

Edit: I managed to cut the release on my macOS box, but I'm still curious about the errors above, if you have any ideas what the potential issue might be.

rimadoma commented 4 years ago

@ctrueden Unable to reproduce on my Win10 laptop. I thought it might be the different number cores available, so I ran the tests repeatedly with Executors.newFixedThreadPool(1..15) (my laptop has 8 cores). They pass. Do to tests still fail if you revert to https://github.com/imagej/imagej-ops/pull/618/commits/365d628b0aba44d28f33777d23e6c37ce1f61560?

Update: I thought it might be something to do with Runtime.getRuntime().availableProcessors(), so I updated my jdk8 and installed openjdk8. Can't reproduce on either.

rimadoma commented 4 years ago

@ctrueden One more thought: is Maven on the Linux box configured to run tests in parallel?

rimadoma commented 4 years ago

I added a commit that uses ThreadService instead of its own ExecutorService. Any difference?