feat(impact-reg): accept a displacement field written as an OME-Zarr store - #68
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe PR adds unified displacement-field loading for ITK files and NGFF OME-Zarr stores, updates registration and uncertainty workflows to preserve and process either representation, and centralizes OME-Zarr metadata and cache handling. Tests cover discovery, copying, replacement, validation, and transform equivalence. ChangesDisplacement-field I/O
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Preset
participant ImpactReg
participant read_displacement_field
participant OMEZarr
Preset->>ImpactReg: produce Moved and DVF by stem
ImpactReg->>OMEZarr: preserve or copy output representation
ImpactReg->>read_displacement_field: read DVF file or store
read_displacement_field-->>ImpactReg: SimpleITK vector field
ImpactReg->>OMEZarr: write compatible averaged DVF
ImpactReg->>ImpactReg: write Transform.h5 from dvf_out
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/impact_reg/impact_reg_konfai/impact_reg.py`:
- Around line 109-116: The displacement-field OME-Zarr round-trip must preserve
the fixed-grid direction matrix alongside origin and spacing. In
apps/impact_reg/impact_reg_konfai/impact_reg.py lines 109-116, update
write_ome_zarr to persist Origin, Spacing, and Direction through the OME-Zarr
attributes contract; in konfai/utils/ITK.py lines 69-72, restore all three
attributes onto the SimpleITK image, including SetDirection(), before
constructing transforms. Update
apps/impact_reg/tests/unit/test_displacement_field_io.py lines 45-50 to use a
non-identity direction matrix and assert it round-trips.
- Around line 247-249: Update uncertainty() to replace both sitk.ReadImage calls
with read_displacement_field(), using the preserved ensemble paths so .ome.zarr
displacement fields are supported. Add a regression test covering uncertainty
calculation with OME-Zarr ensemble members.
In `@konfai/utils/ITK.py`:
- Line 63: Replace the full-volume `read_ome_zarr_data_slice` call in the
surrounding ITK data-processing flow with lazy patch/stream processing; avoid
constructing all-axis `slice(None)` requests or retaining the complete C[Z]YX
field in memory. Use the existing `can_stream_patch` and `read_data_slice`
mechanisms where applicable, processing and writing each patch incrementally
while preserving the current metadata and output behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 57a9d0fa-11ed-4fbc-88c3-6b1a7f063f09
📒 Files selected for processing (3)
apps/impact_reg/impact_reg_konfai/impact_reg.pyapps/impact_reg/tests/unit/test_displacement_field_io.pykonfai/utils/ITK.py
vboussot
force-pushed
the
feat/omezarr-rfc5-impact-reg
branch
from
July 28, 2026 14:32
a946985 to
f7b5514
Compare
vboussot
force-pushed
the
feat/omezarr-rfc5-displacement
branch
from
July 28, 2026 15:10
92571f0 to
8d75f72
Compare
vboussot
force-pushed
the
feat/omezarr-rfc5-impact-reg
branch
from
July 28, 2026 15:10
f7b5514 to
df5c577
Compare
vboussot
force-pushed
the
feat/omezarr-rfc5-impact-reg
branch
from
July 28, 2026 15:50
df5c577 to
2a1d105
Compare
…store
Every step after inference assumed the field was an ITK image called DVF.mha:
discovery, the copy beside the results, the ensemble average, and the Transform.h5
export. A preset emitting an RFC-5 store therefore failed on "Preset inference did
not produce 'DVF.mha'" -- with the store sitting in the directory just listed,
because Path("DVF.ome.zarr").stem is "DVF.ome".
Outputs are now located by NAME and copied in whatever form the preset produced,
a derived (averaged) field is written in the form its members came in, and reading
goes through konfai.utils.ITK.read_displacement_field, which opens either an ITK
image or a store. That reader lives in konfai rather than here: reading a field as
if it were an ordinary image is a failure that does not announce itself -- the
component axis looks like any other, so indexing it returns a third of the field
and a registration that is wrong without looking wrong -- and one place should
know how to open either form.
…OME-Zarr The OME-Zarr write dropped the field's direction matrix and the reader never restored it, so a non-identity fixed-grid direction came back a geometrically wrong transform -- silently, the very failure the store typing is meant to end. Both sides now go through image_to_data / data_to_image, the encoding the Dataset backend already uses, so geometry (Origin, Spacing, Direction) round-trips and neither side re-derives it. The shared geometry builder is lifted out of the Dataset reader into ome_zarr_attributes(). - uncertainty() reads its ensemble members through read_displacement_field too, so an .ome.zarr DVF no longer fails on a raw sitk.ReadImage - _average_displacement accumulates a running sum (two fields at peak, not the whole ensemble stacked) - the round-trip test uses a non-identity Direction, and the suite skips where zarr v3 is unavailable (Python 3.10)
vboussot
force-pushed
the
feat/omezarr-rfc5-impact-reg
branch
from
July 28, 2026 15:53
2a1d105 to
53f58f6
Compare
The guard that turns an unwritable RFC-5 store into a named requirement was asserted nowhere: on zarr 3 the branch is never taken, and on zarr 2 the displacement suite skips wholesale, so it went unverified on every leg of the matrix. Covered beside _require_zarr, whose capability guard is tested the same way, so it runs on 3.10 too -- moving the check below ngff-zarr's writer now fails the suite instead of silently restoring the opaque ValueError. The DIRECTION constant called a rotation matrix "anisotropic" (the spacing is what is anisotropic) and restated the helper docstring beside it.
The OME-Zarr reader memoises NGFF metadata per store path, on the standing assumption that whatever writes a store invalidates it. _copy_output writes one by copying, so a second registration into the same output directory hit the memo and paired the copy's voxels with the replaced store's axes and geometry: a shape mismatch surfaces as a reshape ValueError naming neither the cache nor the store, and equal shapes surface as nothing at all -- the field simply sampled in the wrong place. The invalidation the write paths already performed is now clear_ome_zarr_cache(), which _copy_output calls after replacing a store, and which says what callers outside the module owe it. The regression test uses a store with no konfai sidecar, as a third-party RFC-5 producer emits: konfai's own sidecar is read outside the memo and masks the staleness. Removing the invalidation fails it on geometry, not on a crash.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
tests/unit/test_omezarr_displacement_field.py (1)
36-43: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winScope the Zarr v3 skip to RFC-5-specific tests.
The module-wide mark also skips the plain-image, missing-store, and affine-rejection tests, even though those paths do not require RFC-5 writing. This removes useful coverage on supported Zarr 2 environments; apply the skip only to RFC-5 tests or split the test modules.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit/test_omezarr_displacement_field.py` around lines 36 - 43, Remove the module-wide skipif based on _zarr_v3_available and apply that marker only to the RFC-5 displacement-field tests. Keep plain-image, missing-store, and affine-rejection tests runnable on Zarr 2 environments, either by decorating the RFC-5-specific test functions or separating them into a dedicated module.apps/impact_reg/impact_reg_konfai/impact_reg.py (1)
104-123: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTwo independent implementations of the same OME-Zarr displacement-field write contract. Both sites capture Origin/Spacing/Direction and call
write_ome_zarr(..., displacement_field=True), but they do it separately — the same kind of divergence that caused the earlier "dropped Direction" critical bug this PR fixes.
apps/impact_reg/impact_reg_konfai/impact_reg.py#L104-L123: promote_write_displacement_fieldintokonfai/utils/ITK.pyas a publicwrite_displacement_field(field, dest), symmetric to the existingread_displacement_field, and call it here.konfai/utils/dataset.py#L1363-L1392: reuse the same helper's core serialization (Origin/Spacing/Direction capture) inside thesitk.Transformbranch instead of maintaining a parallel path.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/impact_reg/impact_reg_konfai/impact_reg.py` around lines 104 - 123, Promote _write_displacement_field into public konfai.utils.ITK.write_displacement_field, symmetric with read_displacement_field, and replace the local implementation in apps/impact_reg/impact_reg_konfai/impact_reg.py:104-123 with that helper. In konfai/utils/dataset.py:1363-1392, update the sitk.Transform branch to reuse the helper’s shared OME-Zarr serialization, preserving Origin, Spacing, Direction, and displacement_field=True rather than maintaining a parallel implementation.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/impact_reg/impact_reg_konfai/impact_reg.py`:
- Around line 251-266: Clear existing outputs with the same logical stem before
writing current results in the single-preset and ensemble branches. Update the
cleanup around _copy_output and _write_displacement_field so stale DVF, Moved,
and per-preset ensemble files with different suffixes are removed while
preserving unrelated outputs, then write the current-format files as before.
---
Nitpick comments:
In `@apps/impact_reg/impact_reg_konfai/impact_reg.py`:
- Around line 104-123: Promote _write_displacement_field into public
konfai.utils.ITK.write_displacement_field, symmetric with
read_displacement_field, and replace the local implementation in
apps/impact_reg/impact_reg_konfai/impact_reg.py:104-123 with that helper. In
konfai/utils/dataset.py:1363-1392, update the sitk.Transform branch to reuse the
helper’s shared OME-Zarr serialization, preserving Origin, Spacing, Direction,
and displacement_field=True rather than maintaining a parallel implementation.
In `@tests/unit/test_omezarr_displacement_field.py`:
- Around line 36-43: Remove the module-wide skipif based on _zarr_v3_available
and apply that marker only to the RFC-5 displacement-field tests. Keep
plain-image, missing-store, and affine-rejection tests runnable on Zarr 2
environments, either by decorating the RFC-5-specific test functions or
separating them into a dedicated module.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b4213770-e03d-4744-ae3e-a6a3dc7bf5bb
📒 Files selected for processing (7)
apps/impact_reg/impact_reg_konfai/impact_reg.pyapps/impact_reg/tests/unit/test_displacement_field_io.pykonfai/utils/ITK.pykonfai/utils/dataset.pykonfai/utils/ome_zarr.pytests/unit/test_imaging_formats.pytests/unit/test_omezarr_displacement_field.py
🚧 Files skipped from review as they are similar to previous changes (2)
- konfai/utils/ITK.py
- apps/impact_reg/tests/unit/test_displacement_field_io.py
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@konfai/utils/dataset.py`:
- Around line 356-362: Update the geometry parsing around the scale and
translation mappings to validate that each metadata array has the same length as
axes; raise DatasetManagerError when either length mismatches, before
constructing the mappings. Remove the per-axis 1.0/0.0 fallbacks in the Spacing
and Origin construction so inconsistent NGFF geometry is rejected rather than
fabricated.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 44273b1e-26d3-4fff-a278-2365c6b35c03
📒 Files selected for processing (7)
apps/impact_reg/impact_reg_konfai/impact_reg.pyapps/impact_reg/tests/unit/test_displacement_field_io.pykonfai/utils/ITK.pykonfai/utils/dataset.pykonfai/utils/ome_zarr.pytests/unit/test_imaging_formats.pytests/unit/test_omezarr_displacement_field.py
🚧 Files skipped from review as they are similar to previous changes (4)
- konfai/utils/ITK.py
- tests/unit/test_imaging_formats.py
- apps/impact_reg/impact_reg_konfai/impact_reg.py
- konfai/utils/ome_zarr.py
Comment on lines
+356
to
+362
| scale = dict(zip(axes, metadata.get("scale", []), strict=False)) | ||
| translation = dict(zip(axes, metadata.get("translation", []), strict=False)) | ||
| spatial_axes = [axis for axis in ("x", "y", "z") if axis in axes] | ||
| if "Spacing" not in attributes: | ||
| attributes["Spacing"] = np.asarray([scale.get(axis, 1.0) for axis in spatial_axes]) | ||
| if "Origin" not in attributes: | ||
| attributes["Origin"] = np.asarray([translation.get(axis, 0.0) for axis in spatial_axes]) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the file and inspect the relevant region.
ast-grep outline konfai/utils/dataset.py --view expanded || true
echo '---'
sed -n '300,420p' konfai/utils/dataset.py | cat -n
echo '--- nearby references ---'
rg -n "ome_zarr_attributes|Spacing|Origin|scale|translation|strict=False|DatasetManagerError" konfai/utils/dataset.pyRepository: fideus-labs/KonfAI
Length of output: 16868
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect any tests or callers that constrain OME-Zarr geometry handling.
rg -n "ome_zarr_attributes|Spacing|Origin|translation|scale|OME-Zarr|ngff|zarr" konfai tests .Repository: fideus-labs/KonfAI
Length of output: 50375
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read the OME-Zarr reader/writer path around metadata creation and consumption.
sed -n '1260,1355p' konfai/utils/dataset.py | cat -n
echo '---'
sed -n '1,260p' konfai/utils/ome_zarr.py | cat -n
echo '---'
sed -n '260,420p' konfai/utils/ome_zarr.py | cat -nRepository: fideus-labs/KonfAI
Length of output: 26623
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Probe how ome_zarr_attributes behaves for malformed metadata.
python3 - <<'PY'
from pathlib import Path
import importlib.util
import sys
path = Path("konfai/utils/dataset.py")
spec = importlib.util.spec_from_file_location("dataset_mod", path)
mod = importlib.util.module_from_spec(spec)
spec.loader.exec_module(mod)
cases = [
{
"name": "short scale",
"metadata": {"axes": ["c", "z", "y", "x"], "scale": [1.0, 2.0], "translation": [0.0, 3.0, 4.0, 5.0]},
},
{
"name": "long translation",
"metadata": {"axes": ["c", "z", "y", "x"], "scale": [1.0, 2.0, 3.0, 4.0], "translation": [0.0, 3.0, 4.0, 5.0, 6.0]},
},
{
"name": "missing z scale",
"metadata": {"axes": ["c", "z", "y", "x"], "scale": [1.0, 0.5, 0.6], "translation": [0.0, 1.0, 2.0, 3.0]},
},
]
for case in cases:
attrs = mod.ome_zarr_attributes(case["metadata"])
print(case["name"])
print(" axes:", attrs["OMEAxes"].tolist())
print(" spacing:", attrs.get_np_array("Spacing").tolist())
print(" origin:", attrs.get_np_array("Origin").tolist())
PYRepository: fideus-labs/KonfAI
Length of output: 505
Reject inconsistent NGFF geometry instead of defaulting it. zip(..., strict=False) will silently drop axes when scale or translation is shorter than axes, and the later .get(..., 1.0/0.0) fallback fabricates plausible geometry. Validate the lengths and raise DatasetManagerError on mismatch.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@konfai/utils/dataset.py` around lines 356 - 362, Update the geometry parsing
around the scale and translation mappings to validate that each metadata array
has the same length as axes; raise DatasetManagerError when either length
mismatches, before constructing the mappings. Remove the per-axis 1.0/0.0
fallbacks in the Spacing and Origin construction so inconsistent NGFF geometry
is rejected rather than fabricated.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@konfai/utils/ITK.py`:
- Around line 53-65: The directory-loading branch in the ITK image conversion
flow must validate the OME-Zarr RFC-5 declaration before reading or converting
data. Use the existing is_displacement_field metadata check and raise
TransformError for non-displacement stores before
get_ome_zarr_info/read_ome_zarr_data_slice or data_to_image execute, then add a
regression test covering a normal three-channel OME-Zarr directory.
- Around line 41-65: Redesign read_displacement_field so OME-Zarr inputs are
processed lazily in patches or streamed chunks instead of requesting the full
C[Z]YX volume via read_ome_zarr_data_slice or retaining full-volume copies in
data_to_image and sitk.Cast. Reuse the repository’s
can_stream_patch/read_data_slice mechanisms where applicable, while preserving
the existing SimpleITK-file path and resulting displacement-field geometry and
pixel type.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 98b4c57b-2537-43d7-b014-822c0f3da0d3
📒 Files selected for processing (7)
apps/impact_reg/impact_reg_konfai/impact_reg.pyapps/impact_reg/tests/unit/test_displacement_field_io.pykonfai/utils/ITK.pykonfai/utils/dataset.pykonfai/utils/ome_zarr.pytests/unit/test_imaging_formats.pytests/unit/test_omezarr_displacement_field.py
🚧 Files skipped from review as they are similar to previous changes (4)
- tests/unit/test_imaging_formats.py
- apps/impact_reg/tests/unit/test_displacement_field_io.py
- konfai/utils/dataset.py
- apps/impact_reg/impact_reg_konfai/impact_reg.py
Comment on lines
+41
to
+65
| def read_displacement_field(path: str | Path) -> sitk.Image: | ||
| """A displacement field, from an ITK image file OR an NGFF RFC-5 OME-Zarr store. | ||
|
|
||
| A field written as a store cannot go through ``sitk.ReadImage``, and reading it as an ordinary | ||
| image is worse than failing: the component axis looks like any other, so indexing it yields one | ||
| third of the field and a registration that is wrong without being obviously wrong. This is the one | ||
| place that knows how to open either form, so no caller has to decide. | ||
|
|
||
| The result is ``sitkVectorFloat64`` -- what ``DisplacementFieldTransform`` requires. | ||
| """ | ||
| _require_simpleitk() | ||
| path = Path(path) | ||
| if not path.is_dir(): | ||
| return sitk.ReadImage(str(path), sitk.sitkVectorFloat64) | ||
|
|
||
| from konfai.utils.dataset import data_to_image, ome_zarr_attributes | ||
| from konfai.utils.ome_zarr import get_ome_zarr_info, read_ome_zarr_data_slice | ||
|
|
||
| axes = get_ome_zarr_info(path)["axes"] | ||
| n_axes = 1 + sum(axis in axes for axis in ("z", "y", "x")) # channel-first C[Z]YX | ||
| data, metadata = read_ome_zarr_data_slice(path, tuple(slice(None) for _ in range(n_axes))) | ||
| # Origin, Spacing and Direction together: NGFF scale/translation alone cannot express the | ||
| # direction matrix, so the geometry comes from the konfai sidecar through data_to_image. | ||
| field = data_to_image(data, ome_zarr_attributes(metadata)) | ||
| return sitk.Cast(field, sitk.sitkVectorFloat64) |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major
Do not materialize the entire OME-Zarr field.
Line 61 requests every voxel and then retains additional full-volume buffers through data_to_image and sitk.Cast, which can OOM on clinical DVFs. Redesign this boundary around lazy patch/stream processing rather than constructing the complete C[Z]YX field.
As per coding guidelines, “Never load a full imaging volume into RAM; use lazy, patch-based, or streaming access such as can_stream_patch and read_data_slice.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@konfai/utils/ITK.py` around lines 41 - 65, Redesign read_displacement_field
so OME-Zarr inputs are processed lazily in patches or streamed chunks instead of
requesting the full C[Z]YX volume via read_ome_zarr_data_slice or retaining
full-volume copies in data_to_image and sitk.Cast. Reuse the repository’s
can_stream_patch/read_data_slice mechanisms where applicable, while preserving
the existing SimpleITK-file path and resulting displacement-field geometry and
pixel type.
Source: Coding guidelines
…ut per stem Two holes the RFC-5 declaration was meant to close but nobody consulted. read_displacement_field treated any directory as a field without asking the store what it held, so an ordinary three-channel OME-Zarr image became a working DisplacementFieldTransform -- a registration to nowhere that never failed. It now reads the declaration it was built to trust and raises TransformError. _copy_output removed only the exact path it was about to write, so a re-run emitting the other form left DVF.mha beside DVF.ome.zarr. Discovery is by stem and takes the first match, which sorts to the stale one. Clearing every output of the stem is shared with the ensemble branch through _output_path, and is matched on a literal prefix so a preset name never reads as a glob pattern.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
apps/impact_reg/impact_reg_konfai/impact_reg.py (1)
281-282: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winClear stale
Moved.*outputs in the ensemble path.Line 281 cleans only
DVF.*; the subsequent hard-codedMoved.mhawrite leaves a priorMoved.ome.zarrbehind. This still violates the same-stem replacement contract on a single-preset-to-ensemble rerun.Proposed fix
dvf_out = _output_path(case_out, "DVF", "".join(dvf_paths[0].suffixes)) _write_displacement_field(avg_dvf, dvf_out) transform = sitk.DisplacementFieldTransform(sitk.Cast(avg_dvf, sitk.sitkVectorFloat64)) moving = sitk.ReadImage(str(moving_image)) + moved_out = _output_path(case_out, "Moved", ".mha") sitk.WriteImage( sitk.Resample(moving, avg_dvf, transform, sitk.sitkLinear, 0.0, moving.GetPixelID()), - str(case_out / "Moved.mha"), + str(moved_out), )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/impact_reg/impact_reg_konfai/impact_reg.py` around lines 281 - 282, Update the ensemble output handling around _write_displacement_field to remove stale Moved.* files before writing the hard-coded Moved.mha output, ensuring prior Moved.ome.zarr or other same-stem artifacts are cleared on reruns while preserving the existing DVF cleanup behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@apps/impact_reg/impact_reg_konfai/impact_reg.py`:
- Around line 281-282: Update the ensemble output handling around
_write_displacement_field to remove stale Moved.* files before writing the
hard-coded Moved.mha output, ensuring prior Moved.ome.zarr or other same-stem
artifacts are cleared on reruns while preserving the existing DVF cleanup
behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7b8f049a-493a-4c68-af42-0de937672ecb
📒 Files selected for processing (3)
apps/impact_reg/impact_reg_konfai/impact_reg.pyapps/impact_reg/tests/unit/test_displacement_field_io.pykonfai/utils/ITK.py
🚧 Files skipped from review as they are similar to previous changes (2)
- konfai/utils/ITK.py
- apps/impact_reg/tests/unit/test_displacement_field_io.py
The RFC-5 displacement-field work raised the floor to ngff-zarr>=0.38 in pyproject.toml, but the lock beside it still pinned 0.37.0 — a lock that does not satisfy its own manifest. Anyone installing from it had to re-solve, so the environment stopped being the reproducible one it exists to guarantee. Relocked to 0.40.0 and the transitive versions that came with it. `pixi lock --check` is clean.
The ensemble branch writes Moved.mha verbatim, so a preceding single-preset run whose model emitted a store left Moved.ome.zarr beside it -- the same-stem contract the DVF side had just been given. It goes through _output_path as well. Covered on the helper rather than on one caller, since both writers lean on it, including the assertion that a stem is matched whole: MovedMask.mha shares a prefix with Moved and must survive.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
#67 lets KonfAI write a DVF as an NGFF RFC-5 vector field. The orchestrator could not then consume one: every step after inference assumed an ITK image literally called
DVF.mha— discovery, the copy beside the results, the ensemble average, and theTransform.h5export.A preset emitting a store failed on:
with the store sitting in the directory it had just listed, because
Path("DVF.ome.zarr").stemis"DVF.ome".What
_copy_outputkeeps the form the preset produced (file →copy2, store →copytree), and replaces an existing output of either kind so a re-run overwrites cleanly._write_displacement_fieldwrites a derived (averaged) field in the form its members arrived in, so an ensemble does not silently change format.konfai.utils.ITK.read_displacement_field, which opens an ITK image or a store.That reader lives in
konfai, not here, deliberately: reading a field as if it were an ordinary image is a failure that does not announce itself — the component axis looks like any other, so indexing it returns a third of the field, and a registration that is wrong without looking wrong. One place should know how to open either form.Tests
apps/impact_reg/tests/unit/test_displacement_field_io.py— parametrised over.mhaand.ome.zarr: discovery finds either, the copy preserves the form and overwrites a store, an averaged field stays a declared field, and the transform reads back identically from both.The last one is asserted on a mapped point, not on the array: a field read with its component axis mishandled keeps a plausible shape, and only applying it exposes that.
34 orchestrator unit tests pass;
ruff,ruff format,mypy,banditclean.Summary by CodeRabbit