feat(data)!: field windows measured at run; one Resample, no bound to declare - #95
Conversation
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
apps/impact_reg/tests/unit/test_orchestration.py (1)
248-258: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider covering a group whose entries do not share one form.
This test pins re-staging with one form. The case that is not covered is a group staged with two forms — for example an ensemble whose presets emit
DVF.mhaandDVF.ome.zarr._stage_groupthen labels the root from whichever entry comes last. A test asserting the intended behavior for that input would pin the contract the docstring claims.🤖 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/tests/unit/test_orchestration.py` around lines 248 - 258, The test coverage for _stage_group only exercises repeated staging of one form; add a test that stages a single group containing entries whose sources use different forms, such as DVF.mha and DVF.ome.zarr. Assert the intended root/spec form behavior promised by _stage_group’s docstring, including that the result does not depend incorrectly on whichever entry is processed last.konfai/data/transform.py (1)
1268-1270: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winRelease
_field_windowafter whole-volume sampling. Clear it inResample.__call__after_samplecompletes. Keep it betweenmeasured_region_sourceandstream_region; that streamed path shares the field read. The cached field is float32, not float64.🤖 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/data/transform.py` around lines 1268 - 1270, Update Resample.__call__ to clear self._field_window immediately after _sample completes, before stream_region while preserving it between measured_region_source and stream_region. Keep the existing shared-read behavior for the streamed path and retain the field cache’s float32 representation.
🤖 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 186-195: Update apps/impact_reg/impact_reg_konfai/impact_reg.py
lines 186-195 in _stage_group to collect entry suffixes into a set, reject
groups containing more than one storage form, and derive the returned format
token from the single collected suffix rather than the loop variable. Update
lines 226-234 so each case uses its own staging base, such as work /
"transform_h5" / case, preventing links for different forms from sharing the
same case directory.
- Around line 243-268: Update the function containing the staging write to wrap
the h5py creation and population block in try/except, unlink staging on failure,
then re-raise the original exception; keep os.replace for successful writes.
Revise its memory-usage documentation to state the one-slab peak applies only to
h5 and OME-Zarr inputs, while compressed MetaImage inputs may retain the full
field in resident memory.
- Around line 708-714: Update the transform handling in the evaluation flow
around _stage_group and _run_transform so .h5 inputs are not staged as
directory-backed Dataset roots while still selecting Dataset.H5File; preserve
.itk.txt through the existing SitkFile path, and add explicit handling that
opens the documented Transform.h5 location correctly before evaluation.
In `@docs/source/usage/large-images.md`:
- Line 86: Update the Resample documentation consistently: in
docs/source/usage/large-images.md at lines 86-86, qualify the adjacent
arbitrary-displacement fallback as unbounded, custom, or otherwise refused
displacement; in docs/source/troubleshooting.md at lines 106-106, add bounded,
recorded, or runtime-measured field-backed Resample to the streamable region
list.
In `@konfai/data/transform.py`:
- Around line 1795-1802: Update plan_note so the bound-less displacement note
does not return early before coverage is computed. Preserve that field note and
append or combine the partial-coverage note when applicable, while retaining the
existing disjoint-case handling and returning both notes for cases covering only
part of the target grid.
---
Nitpick comments:
In `@apps/impact_reg/tests/unit/test_orchestration.py`:
- Around line 248-258: The test coverage for _stage_group only exercises
repeated staging of one form; add a test that stages a single group containing
entries whose sources use different forms, such as DVF.mha and DVF.ome.zarr.
Assert the intended root/spec form behavior promised by _stage_group’s
docstring, including that the result does not depend incorrectly on whichever
entry is processed last.
In `@konfai/data/transform.py`:
- Around line 1268-1270: Update Resample.__call__ to clear self._field_window
immediately after _sample completes, before stream_region while preserving it
between measured_region_source and stream_region. Keep the existing shared-read
behavior for the streamed path and retain the field cache’s float32
representation.
🪄 Autofix
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: 720638b9-311c-48cd-9510-f7c0a92112e7
📒 Files selected for processing (33)
apps/impact_reg/impact_reg_konfai/cli.pyapps/impact_reg/impact_reg_konfai/impact_reg.pyapps/impact_reg/impact_reg_konfai/models/convexadam.pyapps/impact_reg/impact_reg_konfai/models/elastix.pyapps/impact_reg/impact_reg_konfai/models/elastix_engine.pyapps/impact_reg/impact_reg_konfai/models/fireants.pyapps/impact_reg/tests/unit/test_displacement_field_io.pyapps/impact_reg/tests/unit/test_orchestration.pydocs/source/concepts/streaming.mddocs/source/config_guide/prediction.mddocs/source/config_guide/transform.mddocs/source/examples/visual-gallery.mddocs/source/reference/components/transforms.mddocs/source/troubleshooting.mddocs/source/usage/large-images.mdkonfai/data/patching.pykonfai/data/transform.pykonfai/transformer.pytests/integration/test_konfai_streamed_prediction.pytests/integration/test_transform_example.pytests/unit/test_api.pytests/unit/test_case_expansion.pytests/unit/test_itk_transforms.pytests/unit/test_resample.pytests/unit/test_resample_to_reference.pytests/unit/test_resample_transform.pytests/unit/test_streamed_read_dispatcher.pytests/unit/test_streamed_write_dispatcher.pytests/unit/test_transform.pytests/unit/test_transform_locality_contract.pytests/unit/test_transformer_workflow.pytests/unit/test_warp.pytests/unit/test_write_pyramid_and_field_bound.py
💤 Files with no reviewable changes (1)
- apps/impact_reg/impact_reg_konfai/cli.py
ed5054d to
57bcf98
Compare
b288f44 to
a26bf5a
Compare
BREAKING. A displacement field no longer needs — or accepts — a declared bound: each region's source window is sized from the field values read at run (one read, shared between sizing and sampling), while the plan stays headers-only and prices the field as zero.
max_displacementis gone, and so is every legacy Resample spelling (Warp,ResampleToReference,ResampleTransform,ResampleToResolution,ResampleToShape) —Resampleis the one name.59fa477feat(data): size a field's windows from the field itself at run9c94f89feat(data): drop max_displacement — the field itself is the bound7a809b3feat(data)!: one Resample, no other spellingd0ae5abfix(impact-reg): one staging root per group, so mixed forms stay readable3a2458cperf(impact-reg): fill Transform.h5 region by regionPublished configs migrate with
s/ResampleToResolution:/Resample:/(and likewise for the other names) at the 1.8 bundle bump.Stacked on
feat/konfai-python-api.