import_sofima_20260912 - #253
Conversation
… logic in the java ImportSofimaClient
…interface and add javadoc comment with reference information to each pipeline step implementation to document the mapping instead
…b-range of z layers from that client
There was a problem hiding this comment.
🟡 Changes recommended
There are a couple of concrete correctness/quality issues to address (misleading CLI parameter description and a driver-side collect() that can cause avoidable memory pressure), and the new pipeline step lacks direct test coverage.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
This PR introduces a new Spark pipeline step for importing SOFIMA displacement fields into Render stacks, and simplifies the alignment pipeline step abstraction by removing per-client “default step id” plumbing in favor of enum-driven step creation.
Changes:
- Added SOFIMA import support end-to-end:
SofimaParameters, a SparkImportSofimaClient, and a newIMPORT_SOFIMApipeline step id. - Simplified
AlignmentPipelineStepby removinggetDefaultStepId()and updating all step clients/tests accordingly. - Minor housekeeping updates (Docker tag comment bump, additional db-dump stage options, small Java 21 stream/list modernizations).
File summaries
| File | Description |
|---|---|
| render-ws-with-mongo-db/Dockerfile | Updates example image tags in comments (1.0.2 → 1.0.3). |
| render-ws-with-mongo-db/db-dump-google-collections.sh | Adds additional supported stage options including SOFIMA/IC3D. |
| render-ws-spark-client/src/test/java/org/janelia/render/client/spark/pipeline/AlignmentPipelineParametersTest.java | Adjusts tests for step-id/interface cleanup and Java 21 list APIs. |
| render-ws-spark-client/src/main/java/org/janelia/render/client/spark/zspacing/ZPositionCorrectionClient.java | Removes default-step-id method; updates step docstring. |
| render-ws-spark-client/src/main/java/org/janelia/render/client/spark/tile/TileIdHackClient.java | Removes default-step-id method; updates step docstring. |
| render-ws-spark-client/src/main/java/org/janelia/render/client/spark/tile/RenderTilesClient.java | Removes default-step-id method; updates step docstring. |
| render-ws-spark-client/src/main/java/org/janelia/render/client/spark/tile/MaskHackClient.java | Removes default-step-id method; updates step docstring. |
| render-ws-spark-client/src/main/java/org/janelia/render/client/spark/ScapeClient.java | Removes default-step-id method; minor Java 21 list usage. |
| render-ws-spark-client/src/main/java/org/janelia/render/client/spark/pipeline/AlignmentPipelineStepId.java | Adds IMPORT_SOFIMA step id mapped to the new Spark client. |
| render-ws-spark-client/src/main/java/org/janelia/render/client/spark/pipeline/AlignmentPipelineStep.java | Removes getDefaultStepId() from the step interface. |
| render-ws-spark-client/src/main/java/org/janelia/render/client/spark/pipeline/AlignmentPipelineParameters.java | Adds SofimaParameters to pipeline parameters and accessor. |
| render-ws-spark-client/src/main/java/org/janelia/render/client/spark/newsolver/DistributedIntensityCorrectionBlockSolverClient.java | Removes default-step-id method; updates step docstring. |
| render-ws-spark-client/src/main/java/org/janelia/render/client/spark/newsolver/DistributedAffineBlockSolverClient.java | Removes default-step-id method; updates step docstring. |
| render-ws-spark-client/src/main/java/org/janelia/render/client/spark/multisem/UnconnectedCrossMFOVClient.java | Removes default-step-id method; stream .toList() modernization. |
| render-ws-spark-client/src/main/java/org/janelia/render/client/spark/multisem/MultiSEMTileRemovalClient.java | Removes default-step-id method; updates step docstring. |
| render-ws-spark-client/src/main/java/org/janelia/render/client/spark/multisem/MFOVMontageMatchPatchClient.java | Removes default-step-id method; updates step docstring. |
| render-ws-spark-client/src/main/java/org/janelia/render/client/spark/multisem/MFOVAsTileClient.java | Removes default-step-id method; updates step docstring. |
| render-ws-spark-client/src/main/java/org/janelia/render/client/spark/multisem/MatchCollectionRenameClient.java | Removes default-step-id method; stream .toList() modernization. |
| render-ws-spark-client/src/main/java/org/janelia/render/client/spark/multisem/LayerAsTileClient.java | Removes default-step-id method; small string check and .toList() modernization. |
| render-ws-spark-client/src/main/java/org/janelia/render/client/spark/multisem/ImportSofimaClient.java | New Spark client implementing the IMPORT_SOFIMA pipeline step. |
| render-ws-spark-client/src/main/java/org/janelia/render/client/spark/multisem/CreepCorrectionSparkClient.java | Removes default-step-id method; updates step docstring. |
| render-ws-spark-client/src/main/java/org/janelia/render/client/spark/multisem/BeamCorrectionSparkClient.java | Removes default-step-id method; updates step docstring. |
| render-ws-spark-client/src/main/java/org/janelia/render/client/spark/MipmapClient.java | Removes default-step-id method; stream .toList() modernization. |
| render-ws-spark-client/src/main/java/org/janelia/render/client/spark/match/MultiStagePointMatchClient.java | Removes default-step-id method; updates step docstring. |
| render-ws-spark-client/src/main/java/org/janelia/render/client/spark/match/CopyMatchClient.java | Removes default-step-id method; updates step docstring. |
| render-ws-spark-client/src/main/java/org/janelia/render/client/spark/match/ClusterCountClient.java | Removes default-step-id method; updates step docstring. |
| render-ws-java-client/src/main/java/org/janelia/render/client/parameter/SofimaParameters.java | New shared parameter bean for SOFIMA import configuration. |
| render-ws-java-client/src/main/java/org/janelia/render/client/multisem/ImportSofimaClient.java | Refactors Java client to use SofimaParameters and derived target stacks. |
Review details
- Files reviewed: 28/28 changed files
- Comments generated: 3
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| @Override | ||
| public void validatePipelineParameters(final AlignmentPipelineParameters pipelineParameters) | ||
| throws IllegalArgumentException { | ||
|
|
||
| final SofimaParameters sofima = pipelineParameters.getSofima(); | ||
|
|
||
| AlignmentPipelineParameters.validateRequiredElementExists("sofima", sofima); | ||
|
|
||
| sofima.validate(); | ||
| } |
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
|
@minnerbe - I went ahead and merged this, but please let me know if you think anything should be changed. Thanks! |
|
I needed to add this to get things working: |
|
Thanks, @trautmane ! The fix looks good to me. I'm a bit concerned about the warnings, especially the one that has a 33px residual. I think the screenshot has all information I need to investigate this. |
|
I investigated the issue and it turned out that the convergence properties of the inversion of the displacement field weren't great. I implemented a Newton method, which should converge much more robustly (getting rid of the warnings above, which indicated that the inversion was wrong). This has been pushed directly to newsolver. |

Added spark client for importing SOFIMA results and cleaned-up AlignmentPipelineStep interface.