-
Notifications
You must be signed in to change notification settings - Fork 33
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Alternating block solve #160
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good - I made some nice-to-have-but-not-essential comments ...
...a-client/src/main/java/org/janelia/render/client/newsolver/DistributedAffineBlockSolver.java
Outdated
Show resolved
Hide resolved
...client/src/main/java/org/janelia/render/client/newsolver/setup/BlockPartitionParameters.java
Show resolved
Hide resolved
.../org/janelia/render/client/newsolver/solvers/AlternatingDomainDecompositionSolverClient.java
Outdated
Show resolved
Hide resolved
.../org/janelia/render/client/newsolver/solvers/AlternatingDomainDecompositionSolverClient.java
Outdated
Show resolved
Hide resolved
This reverts commit 2567a0c. Reason: the results without fixing block boundaries are much better than with fixing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just some minor nits (really :) with one logic change.
render-ws-java-client/src/main/java/org/janelia/render/client/newsolver/setup/RenderSetup.java
Outdated
Show resolved
Hide resolved
render-ws-java-client/src/main/java/org/janelia/render/client/newsolver/setup/RenderSetup.java
Outdated
Show resolved
Hide resolved
.../src/main/java/org/janelia/render/client/newsolver/solvers/ADDIntensityCorrectionClient.java
Show resolved
Hide resolved
...s-java-client/src/main/java/org/janelia/render/client/newsolver/solvers/ADDSolverClient.java
Show resolved
Hide resolved
...src/main/java/org/janelia/render/client/newsolver/solvers/affine/AffineAlignBlockWorker.java
Outdated
Show resolved
Hide resolved
...src/main/java/org/janelia/render/client/newsolver/solvers/affine/AffineAlignBlockWorker.java
Outdated
Show resolved
Hide resolved
...src/main/java/org/janelia/render/client/newsolver/solvers/affine/AffineAlignBlockWorker.java
Show resolved
Hide resolved
...src/main/java/org/janelia/render/client/newsolver/solvers/affine/AffineAlignBlockWorker.java
Outdated
Show resolved
Hide resolved
...src/main/java/org/janelia/render/client/newsolver/solvers/affine/AffineAlignBlockWorker.java
Show resolved
Hide resolved
...src/main/java/org/janelia/render/client/newsolver/solvers/affine/AffineAlignBlockWorker.java
Outdated
Show resolved
Hide resolved
Thanks for the review! I'll try to incorporate your suggestions later today, but if I don't get around to do it and it's time critical, feel free to edit the code yourself. |
Thanks for applying the changes yourself, @trautmane ! I was completely swamped the last days... |
Changes
I added a client that does a couple of rounds of the distributed block solver with alternating layouts to ensure that every tile is in the "interior" of a block at least once. More specifically, the algorithm looks like this:
_runN
)Test
I tested this on a small toy stack (1 mFOV):
Results
Using the error comparison tool, we see that the iterative solution only deviates from the exact one by more than 1.5 px for a handful of tile pairs, all of which contain more or less only resin. Otherwise, the iterative solve looks pretty good to me. In particular, the global coarse block solve has errors well below 1px after the first two runs.
Interestingly, the exact solve seems to have a very visible (and probably wrong) drift in the last few layers, which the iterative solve doesn't.