Projection op improvments - #293
Conversation
Now the project Op can project on a RAI input
Before the changes to the Op, this test failed, because the projection operated on the interval [0, max-min] for an image defined on interval [min, max]. With the changes to the Op, this Op works on [min, max]
6ee3416 to
e59977d
Compare
Only one function call per pixel now
|
I'm happy with the changes made here, just need to confirm they solve the issues described by @andmccall (P.S. sorry for the delayed response on this!) |
There was a problem hiding this comment.
馃煛 Changes recommended
The updated tests and projection ops contain concrete compilation/runtime issues (e.g., invalid interval bounds, use of methods not available on RandomAccessibleInterval, and missing dim validation) that should be fixed before merging.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR updates the transform.project ops to accept projection operations that compute over RandomAccessibleInterval slices (rather than Iterable slices), and adds a function variant that can allocate/translate an output automatically鈥攁ddressing usability concerns raised in #289 and improving correctness for non-zero mins (related to #292).
Changes:
- Replace iterable-based projection with an RAI-slice-based parallel projector (
ProjectParallelComputer), including slice creation viaViews.interval. - Add
ProjectParallelFunctionto exposetransform.projectas aFunctionthat creates and translates its own output. - Expand/modernize projection tests, including interval-bound behavior and function-style invocation.
File summaries
| File | Description |
|---|---|
| scijava-ops-image/src/test/java/org/scijava/ops/image/transform/project/ProjectTest.java | Updates/extends tests for RAI-based projection, interval handling, and function-style usage. |
| scijava-ops-image/src/main/java/org/scijava/ops/image/transform/project/project/ProjectRAIToIterableInterval.java | Removes the older iterable-based projection implementation. |
| scijava-ops-image/src/main/java/org/scijava/ops/image/transform/project/project/ProjectParallelFunction.java | Adds a function-form transform.project that allocates and translates output automatically. |
| scijava-ops-image/src/main/java/org/scijava/ops/image/transform/project/project/ProjectParallelComputer.java | Implements the new RAI-slice-based parallel projection computer and forbids adaptation via hints. |
Review details
Suppressed comments (8)
scijava-ops-image/src/main/java/org/scijava/ops/image/transform/project/project/ProjectParallelComputer.java:80
- Guard against null/negative projection dimensions. As written, a negative dim will bypass the check and then cause an ArrayIndexOutOfBoundsException at min[dim]/max[dim].
scijava-ops-image/src/test/java/org/scijava/ops/image/transform/project/ProjectTest.java:46 - Remove unused imports (Computers, OpBuilder, Random, Function). Leaving them in can fail builds that treat compiler/checkstyle warnings as errors.
This issue also appears on line 123 of the same file.
scijava-ops-image/src/test/java/org/scijava/ops/image/transform/project/ProjectTest.java:123
- After removing the java.util.function.Function import, this Javadoc link won鈥檛 resolve; use a fully qualified reference instead.
scijava-ops-image/src/test/java/org/scijava/ops/image/transform/project/ProjectTest.java:107 - FinalInterval max bounds are inclusive; using 10 for x/y exceeds the 10-pixel (0..9) image extent and can throw when creating the interval view.
scijava-ops-image/src/test/java/org/scijava/ops/image/transform/project/ProjectTest.java:133 - RandomAccessibleInterval does not define getAt(x,y); this test won鈥檛 compile against the declared type. Use a RandomAccess to read pixels instead.
This issue also appears in the following locations of the same file:
- line 161
- line 166
scijava-ops-image/src/test/java/org/scijava/ops/image/transform/project/ProjectTest.java:165
- RandomAccessibleInterval does not have cursor(); iterate via Views.flatIterable(out) (or Views.iterable(out)) instead so this compiles for translated views.
scijava-ops-image/src/test/java/org/scijava/ops/image/transform/project/ProjectTest.java:167 - minAsLongArray()/maxAsLongArray() are not methods on the RandomAccessibleInterval interface; use Intervals.minAsLongArray/maxAsLongArray helpers instead.
scijava-ops-image/src/main/java/org/scijava/ops/image/transform/project/project/ProjectParallelComputer.java:95 - Returning the chunk from forEachChunk() causes LoopBuilder to retain a list of chunk objects that is never used; return null to avoid unnecessary allocations/retention.
- Files reviewed: 4/4 changed files
- Comments generated: 1
- Review effort level: Lite
馃挕 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
Hi @gselzer, Sadly a little busy with many other things at the moment, and the section of code where I discovered this issue ended up not being a useful approach anyway, so I don't have a spot where I can quickly substitute it in. When I have a little more time, I'll write a quick script to verify, but based on the changes, it definitely looks like it should fix the issue. |
Glad to hear I'm not holding you up! In that case no pressing need for you to validate! |
Now we can pass projection functions that operate on RAIs, which makes matching them a little easier. I can imagine pitfalls, though (for example, we could write a
stats.sumOp that works onArrayImgs. If we want to project anArrayImgusing a summation, we'd match that new Op, and then not find atransform.projectOp because the input type of our summation is too specific), so more thought is probably needed.Designed to close #289 and hopefully also #292 after a little more work.