feat(data)!: the transform release line — one Resample, Python workflows, transform backend - #92
Conversation
A geometry vocabulary (grids, boxes, affine maps); a decoder that splits a stored ITK transform into an affine stage and a bounded displacement residual; a torch sampler that applies it on the volume's device. Resample becomes one transform: a target grid — its own, derived from the source by ITK's own arithmetic, or a reference case — reached through the declared map, streamed slab by slab, and it says why when it cannot stream. A map that factorises is read one axis at a time and blended most-shrinking axis first; one that does not goes through grid_sample. A B-spline order with no kernel where the value is built is refused. Docs say what the old names now mean, and the doc harness runs against a real stored transform.
- judge every landing fold on the state the stages before it left - warp the end plane of a B-spline's valid region as ITK warps it - judge coverage through the declared map before refusing a case as disjoint - blend through float32 coordinates whatever the payload dtype - refuse at plan time the map neither route can apply - replay a copy's regions on that copy's own recorded grids - clip to the case's seeded statistic, not the region's own
Streaming is a memory strategy, not a speed strategy: a halo re-reads its overlap, a regrid pulls each slab's window through its map, and a store without bounded region reads decodes the whole volume once per slab — where loading reads the source once. The plan now prices the streamed route (every pending sweep against its own source, headers only) and a case whose working set fits the per-rank budget is LOADED when streaming would read past 1.5x the source. LOAD is a choice, not a fallback. The predictor's streamed-write threshold prices against the config's resolved per-rank budget instead of instantaneous free RAM, so the same case takes the same route on a loaded machine and an idle one. Dataset.bounded_region_reads is the capability both read. Ten degradations answered WHOLE_VOLUME with no reason; each now names the mechanism and what to change, which is what the plan prints the reason for. Statistics stops being whole-volume: it declares GLOBAL_STAT, reads the seeded case statistic first and computes from the tensor only when nothing seeded one. The console speaks only when the run deviates from the printed plan: a designed refusal prints its message and remedy and exits 1 (traceback under KONFAI_DEBUG=1); the chain line carries the stages and the terminal Write destination; one final line states the counts, the wall time, and where outputs.json is; plan.txt keeps the verbose form. Measured: 23 -> 7 console lines on the 6-output run, log 51 -> 18 lines, the refusal 34 -> 6 lines with the remedy last.
- nearest_index, window_index and sampling_dtype move to sampling.py: their only consumers were the two gathers. The dependency arrow is one-way now. - Resample._bound folds its stored stages through bound_of instead of restating the fold inline; composing through the folded affine is interval arithmetic through the product matrix. - Grid.of is from_header plus the refusal, said once. - _DisplacementSource loses its owner/group_keyword parametrisation: Resample is its one owner since the family collapsed. - Dead since the unification, deleted: _halo_from_bound, _array_order_spacing, read_amplification (the route is priced by the plan's own pull maps), and ITK.py's orphaned pre-unification cluster. The docs pages say what the code now does (sampler tolerance band, the LOAD verdict, the env var catalogue). Tests pin the routes that were unpinned: the cost factor monotone in slab fineness, the predictor's stream-worth gate flipping with the config budget, the run handing materialize the plan's route; _sub_cap_sweeps resets per compute_plan. The unreleased changelog section covers the transform release.
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 17 minutes Your organization has reached its usage spending cap. Adjust your spending cap in the billing tab. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (7)
📝 WalkthroughWalkthroughThis PR unifies resampling under ChangesUnified workflow and regridding update
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
tests/unit/test_streamed_write_dispatcher.py (1)
249-276: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winThe comment claims no tolerance, but the assertion still uses
atol=1e-2.Lines 250-251 state that coordinates are global and "There is no tolerance to negotiate here any more". Line 276 still calls
assert_close(got, reference, atol=1e-2, rtol=0). A reader cannot tell whether the tolerance is now vestigial or still required.If the streamed and whole-volume inverses are bit-identical on this path, use
torch.equalas the nearest test does at line 245. If a tolerance is still needed, say what produces it.🤖 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_streamed_write_dispatcher.py` around lines 249 - 276, Align test_stream_rescale_linear_matches_the_whole_volume_inverse_to_float_rounding with its no-tolerance comment: verify whether got and reference are bit-identical on this path and replace the tolerant torch.testing.assert_close check with torch.equal if so. If differences remain, retain an explicit tolerance only after documenting their source in the test comment.konfai/data/patching.py (1)
1466-1493: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winPass the folded per-copy attributes to each draw.
_fold_case_stateupdatesfoldings[index], butstate_initreceivesattributes.DisplacementField._state_initreadsSpacing,Origin, andDirection, so a draw afterResampleorCanonicaluses stale geometry. Use one evolving per-copyAttributefor both paths and preserve draw-time updates.🤖 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/patching.py` around lines 1466 - 1493, Update the expansion loop around _fold_case_state and stage.state_init so each draw receives the corresponding evolving per-copy Attribute from foldings, rather than the stale attributes baseline. Ensure folds update the same Attribute later passed to _state_init, while preserving draw-time attribute changes for subsequent folds and final caching.docs/source/concepts/streaming.md (1)
102-124: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAdd a
REGRIDrow to the locality table and correct the condition count.Two stale spots follow from the
RESCALEtoREGRIDconsolidation.Line 109 names
REGRIDas a streaming region kind, but the table above it has noREGRIDrow. A reader meets the term with no definition. Every other kind named on line 109 has a row.Line 116 says "Seven conditions reject streaming", and the list now holds six items. The previous version had a separate
RESCALErule and a spacing-validation rule; line 123 merged them into oneREGRIDrule.📝 Proposed documentation fix
| `GLOBAL_STAT` | needs whole-volume `Min`/`Max`/`Mean`/`Std` | the statistic once from disk, then the exact patch | +| `REGRID` | resample onto another grid: a change of sampling density, of placement, or both, possibly through a map | the source region the stage declares for the target region, interpolated by the stage | | `SLAB` | a per-voxel value map plus a side effect that needs the slab's place in the volume | nothing on the read path — the dispatcher treats it as `WHOLE_VOLUME`; it streams on the write side (`Mask`, `InferenceStack`) |-Seven conditions reject streaming: +Six conditions reject streaming:🤖 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 `@docs/source/concepts/streaming.md` around lines 102 - 124, Add a REGRID entry to the locality table in the streaming documentation, describing its region-based read behavior consistently with the surrounding locality kinds. Change the rejection heading from “Seven conditions” to “Six conditions” so it matches the six listed rules, without altering the existing conditions.docs/source/config_guide/transform.md (1)
412-418: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winUpdate the stale refusal bullet below this example.
The "What it refuses" list that follows (Lines 429-431) still states that
Resamplerefuses a reference whoseDirectiondiffers from the case's, and tells the reader to runCanonicalfirst. The unifiedResamplenow handles a differing direction as an ordinary rotation;tests/unit/test_resample_to_reference.py::test_a_differing_direction_is_resampled_and_not_refusedpins that behaviour. Remove or rewrite that bullet so the documentation matches the stage.📝 Proposed doc fix
-- a reference whose `Direction` differs from the case's — the map between them - is then a rotation, not a scale and a shift per axis. Reorient first - (`Canonical`); +- nothing about a differing `Direction`: the map between two grids that do not + share their axes is a rotation, and the stage resamples through it in one + interpolation. Reorienting first (`Canonical`) only costs a second pass;🤖 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 `@docs/source/config_guide/transform.md` around lines 412 - 418, Update the “What it refuses” list in the Resample documentation to remove or rewrite the bullet claiming differing reference and case Direction values are refused or require Canonical first. Ensure it reflects that Resample handles differing directions as ordinary rotations, consistent with test_a_differing_direction_is_resampled_and_not_refused.konfai/data/transform.py (1)
591-621: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winHandle a seeded statistic stored as a one-element array.
float(cache_attribute["Min"])assumes the seed is a bare scalar.Attribute.__getitem__returns text, so a seed written asnp.asarray([value])reads back as"[0.0]"andfloat()raisesValueError.Statistics.__call__in this same file (Lines 3374-3380) states that a seeded statistic arrives "as a bare scalar or a one-element array, depending on who seeded it", and it handles both forms. Apply the same handling here soClipcannot crash on a seed written by another producer.🛡️ Proposed fix reusing the two-form read
+ `@staticmethod` + def _seeded(cache_attribute: Attribute, key: str) -> float: + # Same two forms Statistics reads: a bare scalar, or a one-element array. + try: + return float(cache_attribute[key]) + except (TypeError, ValueError): + return float(cache_attribute.get_tensor(key).reshape(-1)[0])if self.mask is None and "StatisticsSeeded" in cache_attribute and "Min" in cache_attribute: - min_value = float(cache_attribute["Min"]) + min_value = self._seeded(cache_attribute, "Min") else: min_value = torch.min(tensor_masked)if self.mask is None and "StatisticsSeeded" in cache_attribute and "Max" in cache_attribute: - max_value = float(cache_attribute["Max"]) + max_value = self._seeded(cache_attribute, "Max") else: max_value = torch.max(tensor_masked)🤖 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 591 - 621, Update the seeded Min and Max handling in the Clip transformation to accept cache attributes serialized as either bare scalars or one-element arrays. Reuse the existing two-form parsing behavior from Statistics.__call__ rather than calling float() directly on the raw Attribute.__getitem__ text, while preserving the current tensor fallback when no seeded statistic is available.
🧹 Nitpick comments (8)
tests/unit/test_resample_sampler_rules.py (1)
172-192: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTest cropped source windows with
nearestmode.Lines 182 and 186 execute only the linear
grid_samplepath.nearestuses separate index andwindow_indexlogic. Add the same global-coordinate assertion fornearestso a label-map offset regression cannot pass this test suite.Proposed test extension
-def test_a_region_reads_the_same_voxels_as_the_whole_volume() -> None: +@pytest.mark.parametrize(("mode", "relative_atol"), [("linear", 1e-5), ("nearest", 0.0)]) +def test_a_region_reads_the_same_voxels_as_the_whole_volume(mode: str, relative_atol: float) -> None: ... - whole = gather(volume, _coordinates(), [0, 0, 0], list(_SOURCE), "linear", 0.0) + whole = gather(volume, _coordinates(), [0, 0, 0], list(_SOURCE), mode, 0.0) ... - partial = gather(window, _coordinates(), start, list(_SOURCE), "linear", 0.0) + partial = gather(window, _coordinates(), start, list(_SOURCE), mode, 0.0) ... - torch.testing.assert_close(partial[reach], whole[reach], rtol=0, atol=1e-5 * span) + torch.testing.assert_close(partial[reach], whole[reach], rtol=0, atol=relative_atol * span)🤖 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_resample_sampler_rules.py` around lines 172 - 192, Extend test_a_region_reads_the_same_voxels_as_the_whole_volume to cover nearest mode using the same whole-volume and cropped-window setup. Call gather with "nearest" for both inputs and assert the corresponding in-window region matches exactly, preserving the existing linear-mode coverage.tests/unit/test_warp.py (1)
227-245: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueStale
halowording in the docstring.Lines 231-232 still describe the bound as deriving a halo ("the way the halo is derived", "that axis is the one whose halo was too small").
Warpnow declaresREGRIDand sizes a geometry-derived source window, as the sibling tests at lines 94-96 state. Update the wording to "window" so the file uses one term for the concept.🤖 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_warp.py` around lines 227 - 245, The docstring of test_a_field_beyond_the_declared_bound_raises uses stale “halo” terminology; update both references to “window” to match the geometry-derived source window terminology used by Warp and sibling tests, without changing the test behavior.tests/unit/test_streamed_read_dispatcher.py (1)
272-297: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd a route-independent assertion so a shared regression cannot pass.
referenceat line 288 is produced by running the sameResamplestages the manager runs, andmanager.shapes[0]is[8, 10, 10]because a 1.5 → 3.0 → 1.5 round trip returns to the source extent. If both stages regressed to a pass-through, the shape assertion and thetorch.equalcomparison would both still hold, and the docstring's regression would go unnoticed.Pin the property directly: after the round trip through 3.0 mm the data must have lost detail, so the result must differ from the input volume.
💚 Proposed extra assertion
manager.load(manager.transforms, []) reference = _fresh_chain_reference(volume, chain(), manager.dataset._attributes()) assert torch.equal(manager.data[0], reference) + # A pass-through second stage would return the source untouched: the down-then-up trip must not. + assert not torch.equal(reference, torch.from_numpy(volume.copy()))🤖 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_streamed_read_dispatcher.py` around lines 272 - 297, Add a direct assertion in test_a_second_resample_reads_the_first_ones_grid that the round-trip result manager.data[0] differs from the original volume, ensuring the resampling stages do not both pass through input unchanged while preserving the existing reference and shape checks.konfai/data/patching.py (1)
1746-1756: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUpdate the refusal list: the
Resampletype check no longer exists.Lines 1752-1753 still say a
REGRID"without a knownSpacing(or that is not a :class:Resample)" rejects streaming. The dedicatedRESCALEbranch that made that check was removed, and the planner now treats every region kind uniformly: aREGRIDstage refuses only through its ownpatch_localitydeclaration. Drop the parenthetical so the docstring states the current contract.🤖 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/patching.py` around lines 1746 - 1756, Update the streaming refusal list in the relevant patching planner docstring to remove the obsolete “or that is not a Resample” condition. Keep the existing requirement for known REGRID Spacing and state that REGRID rejection otherwise follows its own patch_locality declaration.konfai/predictor.py (1)
967-995: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsolidate the duplicated remap and shape logic in the two region branches.
Lines 972-981 and 983-990 repeat the same forward/inverted dispatch: pick
stream_region_targetorstream_region_sourcefor the pull, theninverse_transform_shapeortransform_shapefor the out shape. Only two things differ: theREGRIDbranch also states the attribute transition (inverse_stream_cache_attribute/write_stream_cache_attribute) and skips the one-voxel probe run. Extracting the shared dispatch into a small helper would remove the risk of the two copies drifting when a new region kind is added.🤖 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/predictor.py` around lines 967 - 995, Consolidate the duplicated forward/inverted remap and shape dispatch used by the REGRID and _REGION_KINDS branches into a small helper, anchored to the existing _RemapPull and transform_shape calls. Have the helper select the appropriate pull function and output shape based on stage.inverted, while keeping REGRID-specific cache-attribute updates and probe-skipping behavior in the caller. Replace both duplicated blocks with the helper and preserve their existing shape accumulation.konfai/data/data_manager.py (1)
86-91: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider renaming
_format_gib.The helper no longer always formats GiB. It now selects TiB, GiB, MiB, KiB, or B. The name misleads a reader at every call site, and
konfai/transformer.pyimports it and uses it in about ten plan and error messages.
_format_bytesstates what the function does.The behaviour itself is correct, including the sign handling:
abs(num_bytes)selects the unit and the formatted value keeps the sign.🤖 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/data_manager.py` around lines 86 - 91, Rename the helper _format_gib to _format_bytes and update all imports and call sites, including those in transformer.py and data_manager.py. Preserve the existing unit-selection logic, sign handling, and formatting behavior unchanged.konfai/data/sampling.py (1)
157-160: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winConsider caching the device copy of
stage.values.
_displacement_atcallstorch.tensor(stage.values, ...)on every invocation. On the streamed path this runs once per region, so a dense displacement field is re-copied to the device for every slab. For a(3, Z, Y, X)float64 field that is the largest allocation in the loop, and it is identical across regions of the same case.
DisplacementStageis a frozen dataclass, so a memo keyed by(id(stage), device)in this module, or a small cache on the caller that builds the stages, keeps the copy once per case.Also prefer
torch.as_tensor(...)overtorch.tensor(...)here;torch.tensoralways copies, whileas_tensorcan reuse the NumPy buffer on CPU.🤖 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/sampling.py` around lines 157 - 160, Update _displacement_at to avoid recreating the device tensor from stage.values on every call: cache the converted values per DisplacementStage and device, and retrieve that cached tensor for subsequent regions. Use torch.as_tensor with the existing dtype and device arguments so CPU-backed buffers can be reused while preserving the current tensor shape and behavior.konfai/data/transform.py (1)
1579-1601: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueName the two
Sizepushes.Lines 1600 and 1601 assign
cache_attribute["Size"]twice.Attribute.__setitem__stacks the values, so this pushes the source size under the target size, and_inverse_geometrypops both. The intent is correct but reads as a redundant assignment. Add a comment that states the stack, as the geometry-key loop above already does.📝 Proposed comment
+ # A stack, not a duplicate: the source size goes under the target's, and ``inverse`` pops + # both -- the first to discard the grid it is leaving, the second to learn what to restore. cache_attribute["Size"] = np.asarray(shape) cache_attribute["Size"] = np.asarray([int(extent) for extent in target.size_zyx])🤖 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 1579 - 1601, Add a concise comment between the two cache_attribute["Size"] assignments in write_stream_cache_attribute explaining that Attribute.__setitem__ stacks the source size beneath the target size, and that _inverse_geometry pops both in reverse order. Preserve both assignments unchanged.
🤖 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 `@docs/source/concepts/streaming.md`:
- Around line 269-272: Update the streaming concepts documentation paragraph to
include REGRID among the region kinds that must implement stream_region_source,
alongside ORIENTATION and CROP. Correct the HALO sentence to use singular
agreement, replacing “their regions” with wording appropriate for HALO.
In `@docs/source/reference/environment.md`:
- Around line 74-75: Split the environment-variable documentation so
KONFAI_DEBUG_LAST_LAYER has its own bullet immediately after the KONFAI_DEBUG
entry, preserving the existing KONFAI_DEBUG description and documenting the
second variable as a separate list item.
In `@docs/source/usage/large-images.md`:
- Around line 199-205: Update the documentation around the fused-blend accuracy
statement to remove the claim that argmax labels absorb ~1e-5 interpolation
differences. State that such differences can change argmax when logits are
close, and distinguish exact nearest-neighbor resampling of already-argmaxed
labels from linear logit resampling followed by argmax.
In `@konfai/data/patching.py`:
- Around line 2876-2890: The _refold_copy_records method currently refolds and
recomputes state for every patch. Cache the folded state keyed by stream_source
and copy a, reuse it when both are unchanged, and refold only when either
changes; also guard that stage_plans is non-empty before accessing
stage_plans[0].in_shape.
In `@konfai/transformer.py`:
- Around line 142-155: Update Transformer.report’s estimated condition to
include plans containing LOAD entries, so LOAD-only nonverbose reports retain
the estimator caveat about assumed dtype/channels. Add a report test covering a
LOAD-only plan and verifying the caveat is present.
In `@konfai/utils/ITK.py`:
- Around line 251-272: Update decode_transform_stages for CompositeTransform
members to dispatch on each member’s GetName(), downcast or reconstruct the
corresponding BSplineTransform or DisplacementFieldTransform, then pass it to
the existing type-specific decoding logic. Preserve reverse stage ordering and
add a regression test covering composite round-tripping with non-linear members.
In `@tests/unit/test_sampling.py`:
- Around line 214-225: Update test_nearest_is_byte_identical_on_a_label_map to
compare against ITK’s per-voxel nearest-neighbor oracle by resampling with a
zero displacement field, as done in the sibling test in
test_resample_to_reference.py. Keep the existing image, transform, KonfAI
sampling region, and exact array comparison unchanged; replace only the
linear-transform _resample_whole oracle.
In `@tests/unit/test_streamed_read_dispatcher.py`:
- Around line 623-647: Update
test_the_read_factor_grows_as_the_budget_cuts_finer_slabs to explicitly require
that _SWEEP_SLAB_ROWS is at least the volume height of 32 rows before asserting
factors[0] is approximately 1.0, or derive the test volume height from that cap.
Keep the monotonicity and final amplification assertions unchanged.
In `@tests/unit/test_streamed_write_dispatcher.py`:
- Around line 636-656: Update
test_the_stream_worth_gate_prices_the_config_budget_not_the_machine to accept
monkeypatch and delete KONFAI_STREAM_WORTH_THRESHOLD before invoking
_worth_streaming, ensuring the test uses _STREAM_WORTH_MIN_FRACTION. Also cover
the documented no-budget path by setting _per_rank_budget_bytes to None and
asserting its auto-budget fraction behavior, or remove that claim from the
docstring if it is not intended to be tested.
In `@tests/unit/test_transform_locality_contract.py`:
- Line 41: Replace the module-level SimpleITK import with
pytest.importorskip("SimpleITK") assigned to sitk, placing it after the pytest
import so tests are skipped cleanly when the optional dependency is unavailable.
In `@tests/unit/test_warp.py`:
- Around line 155-161: Correct the explanatory comment above the locality
assertion to state that the window grows by 3, 6, and 0.5 voxels for the (z, y,
x) axes, matching the reaches tuple used below; leave the test logic unchanged.
In `@tests/unit/test_write_pyramid_and_field_bound.py`:
- Around line 194-201: Update tmp_field_store to accept pytest’s tmp_path
fixture instead of creating a directory with tempfile.mkdtemp(); construct the
fields path beneath tmp_path, preserving the existing OME-Zarr store layout and
return value so pytest manages cleanup.
---
Outside diff comments:
In `@docs/source/concepts/streaming.md`:
- Around line 102-124: Add a REGRID entry to the locality table in the streaming
documentation, describing its region-based read behavior consistently with the
surrounding locality kinds. Change the rejection heading from “Seven conditions”
to “Six conditions” so it matches the six listed rules, without altering the
existing conditions.
In `@docs/source/config_guide/transform.md`:
- Around line 412-418: Update the “What it refuses” list in the Resample
documentation to remove or rewrite the bullet claiming differing reference and
case Direction values are refused or require Canonical first. Ensure it reflects
that Resample handles differing directions as ordinary rotations, consistent
with test_a_differing_direction_is_resampled_and_not_refused.
In `@konfai/data/patching.py`:
- Around line 1466-1493: Update the expansion loop around _fold_case_state and
stage.state_init so each draw receives the corresponding evolving per-copy
Attribute from foldings, rather than the stale attributes baseline. Ensure folds
update the same Attribute later passed to _state_init, while preserving
draw-time attribute changes for subsequent folds and final caching.
In `@konfai/data/transform.py`:
- Around line 591-621: Update the seeded Min and Max handling in the Clip
transformation to accept cache attributes serialized as either bare scalars or
one-element arrays. Reuse the existing two-form parsing behavior from
Statistics.__call__ rather than calling float() directly on the raw
Attribute.__getitem__ text, while preserving the current tensor fallback when no
seeded statistic is available.
In `@tests/unit/test_streamed_write_dispatcher.py`:
- Around line 249-276: Align
test_stream_rescale_linear_matches_the_whole_volume_inverse_to_float_rounding
with its no-tolerance comment: verify whether got and reference are
bit-identical on this path and replace the tolerant torch.testing.assert_close
check with torch.equal if so. If differences remain, retain an explicit
tolerance only after documenting their source in the test comment.
---
Nitpick comments:
In `@konfai/data/data_manager.py`:
- Around line 86-91: Rename the helper _format_gib to _format_bytes and update
all imports and call sites, including those in transformer.py and
data_manager.py. Preserve the existing unit-selection logic, sign handling, and
formatting behavior unchanged.
In `@konfai/data/patching.py`:
- Around line 1746-1756: Update the streaming refusal list in the relevant
patching planner docstring to remove the obsolete “or that is not a Resample”
condition. Keep the existing requirement for known REGRID Spacing and state that
REGRID rejection otherwise follows its own patch_locality declaration.
In `@konfai/data/sampling.py`:
- Around line 157-160: Update _displacement_at to avoid recreating the device
tensor from stage.values on every call: cache the converted values per
DisplacementStage and device, and retrieve that cached tensor for subsequent
regions. Use torch.as_tensor with the existing dtype and device arguments so
CPU-backed buffers can be reused while preserving the current tensor shape and
behavior.
In `@konfai/data/transform.py`:
- Around line 1579-1601: Add a concise comment between the two
cache_attribute["Size"] assignments in write_stream_cache_attribute explaining
that Attribute.__setitem__ stacks the source size beneath the target size, and
that _inverse_geometry pops both in reverse order. Preserve both assignments
unchanged.
In `@konfai/predictor.py`:
- Around line 967-995: Consolidate the duplicated forward/inverted remap and
shape dispatch used by the REGRID and _REGION_KINDS branches into a small
helper, anchored to the existing _RemapPull and transform_shape calls. Have the
helper select the appropriate pull function and output shape based on
stage.inverted, while keeping REGRID-specific cache-attribute updates and
probe-skipping behavior in the caller. Replace both duplicated blocks with the
helper and preserve their existing shape accumulation.
In `@tests/unit/test_resample_sampler_rules.py`:
- Around line 172-192: Extend
test_a_region_reads_the_same_voxels_as_the_whole_volume to cover nearest mode
using the same whole-volume and cropped-window setup. Call gather with "nearest"
for both inputs and assert the corresponding in-window region matches exactly,
preserving the existing linear-mode coverage.
In `@tests/unit/test_streamed_read_dispatcher.py`:
- Around line 272-297: Add a direct assertion in
test_a_second_resample_reads_the_first_ones_grid that the round-trip result
manager.data[0] differs from the original volume, ensuring the resampling stages
do not both pass through input unchanged while preserving the existing reference
and shape checks.
In `@tests/unit/test_warp.py`:
- Around line 227-245: The docstring of
test_a_field_beyond_the_declared_bound_raises uses stale “halo” terminology;
update both references to “window” to match the geometry-derived source window
terminology used by Warp and sibling tests, without changing the test behavior.
🪄 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: ee91e969-7130-45d4-b7dd-062796467592
📒 Files selected for processing (41)
AGENTS.mdCHANGELOG.mddocs/source/concepts/streaming.mddocs/source/config_guide/transform.mddocs/source/reference/api/extension-points.mddocs/source/reference/components/transforms.mddocs/source/reference/environment.mddocs/source/usage/large-images.mdexamples/Transform/README.mdexamples/Transform/Transform.ymlkonfai/data/augmentation.pykonfai/data/case_reduction.pykonfai/data/data_manager.pykonfai/data/geometry.pykonfai/data/patching.pykonfai/data/sampling.pykonfai/data/transform.pykonfai/predictor.pykonfai/transformer.pykonfai/utils/ITK.pykonfai/utils/dataset.pykonfai/utils/runtime.pytests/integration/test_konfai_streamed_prediction.pytests/integration/test_transform_doc_examples.pytests/unit/conftest.pytests/unit/test_case_expansion.pytests/unit/test_geometry.pytests/unit/test_packaging.pytests/unit/test_resample.pytests/unit/test_resample_sampler_rules.pytests/unit/test_resample_to_reference.pytests/unit/test_resample_transform.pytests/unit/test_sampling.pytests/unit/test_streamed_read_dispatcher.pytests/unit/test_streamed_write_dispatcher.pytests/unit/test_transform.pytests/unit/test_transform_bound.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)
- docs/source/reference/api/extension-points.md
| def _refold_copy_records(self, a: int, stream_source: _PatchStreamSource) -> None: | ||
| """Re-fold copy ``a``'s chain state before replaying a region of it. | ||
|
|
||
| A stage keys its per-case records by the CASE name — a stored transform is looked up by | ||
| it — so the copies of an Expand share one key and the last WALK's records win. The write | ||
| sweeps re-plan before sweeping and the whole-volume path re-records at call time; the | ||
| patch replay is the consumer left over, and reading two copies interleaved would otherwise | ||
| hand one copy the other's grids. Headers only — no voxel is read. | ||
| """ | ||
| if not stream_source.stages: | ||
| return | ||
| shape = list(stream_source.stage_plans[0].in_shape) | ||
| state = Attribute(self.cache_attributes_bak[a]) | ||
| for stage in stream_source.stages: | ||
| shape = self._fold_case_state(stage, shape, state) |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check whether Resample.transform_shape memoises per-case grids and stored transforms.
set -euo pipefail
ast-grep run --pattern $'def transform_shape($$$) {
$$$
}' --lang python konfai/data/transform.py 2>/dev/null || true
rg -nP --type=py -B 3 -A 30 'class Resample\b' konfai/data/transform.py | head -60
rg -nP --type=py -C 6 '_grids|_stored\b|_probed' konfai/data/transform.pyRepository: fideus-labs/KonfAI
Length of output: 14238
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- transform_shape and nearby methods ---'
rg -n -A 55 -B 12 'def transform_shape\b|def _fold_case_state\b|def _refold_copy_records\b' konfai/data/transform.py konfai/data/patching.py
printf '%s\n' '--- streamed region call path ---'
rg -n -A 45 -B 20 '_get_streamed_region_data\b|_get_streamed_data\b|_refold_copy_records\b' konfai/data/patching.py
printf '%s\n' '--- stream source construction and plan invariants ---'
rg -n -A 35 -B 20 '_PatchStreamSource|stage_plans|_plan_stream_region|_patch_stream_sources' konfai/data/patching.pyRepository: fideus-labs/KonfAI
Length of output: 50375
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- streamed read entry and refold call ---'
sed -n '2780,2898p' konfai/data/patching.py
printf '%s\n' '--- replay implementation tail ---'
sed -n '2892,3005p' konfai/data/patching.py
printf '%s\n' '--- source type and construction ---'
rg -n -A 30 -B 12 'class _PatchStreamSource|_PatchStreamSource\(' konfai/data/patching.py
printf '%s\n' '--- plan construction and refusal paths ---'
rg -n -A 55 -B 20 'def _plan_stream_region|stage_plans=|stages=' konfai/data/patching.pyRepository: fideus-labs/KonfAI
Length of output: 28175
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
import ast
from pathlib import Path
path = Path("konfai/data/patching.py")
tree = ast.parse(path.read_text())
def find(name):
for node in ast.walk(tree):
if isinstance(node, (ast.FunctionDef, ast.AsyncFunctionDef)) and node.name == name:
return node
raise SystemExit(f"missing {name}")
for name in ("_plan_stream_region", "_get_streamed_region_data", "_get_streamed_data", "_refold_copy_records"):
node = find(name)
calls = [
ast.unparse(child)
for child in ast.walk(node)
if isinstance(child, ast.Call)
]
print(f"{name}:")
for call in calls:
if "_refold_copy_records" in call or "plans.append" in call or "return refuse" in call:
print(f" {call}")
plan = find("_plan_stream_region")
print("plan tuple-return shapes:")
for node in ast.walk(plan):
if isinstance(node, ast.Return):
value = ast.unparse(node.value) if node.value else "None"
if "plans" in value or "refuse" in value:
print(f" {value}")
PY
printf '%s\n' '--- plan loop and completion ---'
sed -n '1760,1845p' konfai/data/patching.pyRepository: fideus-labs/KonfAI
Length of output: 6693
Avoid refolding the chain for every patch
Resample caches stored transforms by case, but _refold_copy_records still re-records grids and recomputes the target grid for every region patch. Cache the folded state per stream source and copy, and refold only when either changes. Guard stage_plans before indexing it.
🤖 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/patching.py` around lines 2876 - 2890, The _refold_copy_records
method currently refolds and recomputes state for every patch. Cache the folded
state keyed by stream_source and copy a, reuse it when both are unchanged, and
refold only when either changes; also guard that stage_plans is non-empty before
accessing stage_plans[0].in_shape.
7fb4931 to
4f90108
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/unit/test_resample_sampler_rules.py (1)
83-88: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winThe dtype assertion is now tautological.
Line 83 calls
.float()on the result, soassert guarded.dtype is torch.float32at Line 86 cannot fail.gathercasts back to the source dtype, so the sampler returns half here and only the explicit.float()makes the assertion true.The drift check at Line 87-88 still tests the accumulation rule. Assert the sampler's own return dtype separately, so the line means something again.
💚 Proposed fix
volume = _volume(offset=2050.0) # 2050..2450, entirely above 2048, where float16 spacing is 2 - guarded = _sample(torch.from_numpy(volume).half()).float() + sampled = _sample(torch.from_numpy(volume).half()) + guarded = sampled.float() reference = _sample(torch.from_numpy(volume)) - assert guarded.dtype is torch.float32 + assert sampled.dtype is torch.float16, "the store's dtype is the store's" drift = float((guarded - reference).abs().max())🤖 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_resample_sampler_rules.py` around lines 83 - 88, Update the test around _sample so it captures the sampler result before converting it with .float(), then assert that raw result has the expected sampler return dtype. Keep the explicit float conversion only for the drift comparison, preserving the existing accumulation-drift check.
🧹 Nitpick comments (2)
konfai/data/transform.py (1)
1611-1612: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument the stacked
Sizewrite.Two consecutive assignments to
"Size"read as a dead first write. They are not:Attribute.__setitem__stacks a bare key, so Line 1611 pushes the SOURCE shape and Line 1612 pushes the TARGET size on top of it._inverse_geometry(Line 1721-1722) depends on exactly that pairing — it pops the target and keeps the source.Every other subtlety in this class carries a comment. Add one here, or a reader deduplicating the two lines breaks
inversesilently.♻️ Proposed comment
- cache_attribute["Size"] = np.asarray(shape) - cache_attribute["Size"] = np.asarray([int(extent) for extent in target.size_zyx]) + # STACKED, not overwritten: a bare key pushes (see Attribute.__setitem__). The source shape + # goes under the target's so ``_inverse_geometry`` can pop the target and restore the source. + cache_attribute["Size"] = np.asarray(shape) + cache_attribute["Size"] = np.asarray([int(extent) for extent in target.size_zyx])🤖 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 1611 - 1612, Add an inline comment between the consecutive `cache_attribute["Size"]` assignments explaining that `Attribute.__setitem__` stacks bare-key writes: the first stores the source shape, the second stores the target size, and `_inverse_geometry` relies on popping them as a pair. Preserve both assignments unchanged.konfai/transformer.py (1)
463-464: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDo not reach into
DatasetManager._sweep_rowsfrom the planner.Every other manager call in
_routeuses a public method (predicted_stream_read_factor, andset_memory_budget,peak_case_bytes,stream_refusalelsewhere). Line 463 calls the private_sweep_rowsand rebuilds its arguments here, so the planner now depends on that method's signature and on the meaning ofshapes[0]/base_shape[0].Expose the question the planner is actually asking as a public predicate on
DatasetManager, in the same wayset_memory_budgetwas made public for the same reason.♻️ Proposed shape
- if manager._sweep_rows(list(manager.shapes[0]), int(manager.base_shape[0])) < _SWEEP_SLAB_ROWS: + if manager.sweeps_below_default_slab(): self._sub_cap_sweeps = TrueIn
konfai/data/patching.py, onDatasetManager:def sweeps_below_default_slab(self) -> bool: """Whether the budget lowers this case's sweep slab under the default height. Public because the planner has to say so in its notes, and the rows are the manager's own arithmetic -- the same reason set_memory_budget is public. """ return self._sweep_rows(list(self.shapes[0]), int(self.base_shape[0])) < _SWEEP_SLAB_ROWS🤖 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/transformer.py` around lines 463 - 464, Replace the planner’s direct _sweep_rows call in _route with a public DatasetManager predicate named sweeps_below_default_slab. Implement that method on DatasetManager using the manager-owned sweep calculation and _SWEEP_SLAB_ROWS threshold, then use its boolean result to set _sub_cap_sweeps without rebuilding shapes or base_shape arguments in the planner.
🤖 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/data/transform.py`:
- Around line 568-578: Make bound matching consistent between the
locality-planning logic and __call__: update the string checks in the shown
stat_keys loop to use the same exact `"min"`/`"max"` comparisons as __call__, so
uppercase spellings are not classified as seeded statistics. Preserve the
existing whole-volume fallback for other string bounds.
- Around line 1013-1018: Update Resample’s __init__ validation to raise
TransformError when any spacing or shape component is negative, rather than
clamping values to zero. Preserve non-negative values and the existing needs
calculation, and ensure the error identifies the invalid argument and how to
correct it.
In `@konfai/utils/ITK.py`:
- Around line 251-257: Pin the SimpleITK dependency to a minimum version that
provides Transform.Downcast(), using the project’s dependency configuration, so
the CompositeTransform handling in decode_transform_stages can safely call
Downcast().
---
Outside diff comments:
In `@tests/unit/test_resample_sampler_rules.py`:
- Around line 83-88: Update the test around _sample so it captures the sampler
result before converting it with .float(), then assert that raw result has the
expected sampler return dtype. Keep the explicit float conversion only for the
drift comparison, preserving the existing accumulation-drift check.
---
Nitpick comments:
In `@konfai/data/transform.py`:
- Around line 1611-1612: Add an inline comment between the consecutive
`cache_attribute["Size"]` assignments explaining that `Attribute.__setitem__`
stacks bare-key writes: the first stores the source shape, the second stores the
target size, and `_inverse_geometry` relies on popping them as a pair. Preserve
both assignments unchanged.
In `@konfai/transformer.py`:
- Around line 463-464: Replace the planner’s direct _sweep_rows call in _route
with a public DatasetManager predicate named sweeps_below_default_slab.
Implement that method on DatasetManager using the manager-owned sweep
calculation and _SWEEP_SLAB_ROWS threshold, then use its boolean result to set
_sub_cap_sweeps without rebuilding shapes or base_shape arguments in the
planner.
🪄 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: 333bec9c-141c-4b3a-8dfd-fa9525800fd8
📒 Files selected for processing (42)
AGENTS.mdCHANGELOG.mddocs/source/concepts/streaming.mddocs/source/config_guide/transform.mddocs/source/reference/api/extension-points.mddocs/source/reference/components/transforms.mddocs/source/reference/environment.mddocs/source/usage/large-images.mdexamples/Transform/README.mdexamples/Transform/Transform.ymlkonfai-mcp/konfai_mcp/catalog.pykonfai/data/augmentation.pykonfai/data/case_reduction.pykonfai/data/data_manager.pykonfai/data/geometry.pykonfai/data/patching.pykonfai/data/sampling.pykonfai/data/transform.pykonfai/predictor.pykonfai/transformer.pykonfai/utils/ITK.pykonfai/utils/dataset.pykonfai/utils/runtime.pytests/integration/test_konfai_streamed_prediction.pytests/integration/test_transform_doc_examples.pytests/unit/conftest.pytests/unit/test_case_expansion.pytests/unit/test_geometry.pytests/unit/test_packaging.pytests/unit/test_resample.pytests/unit/test_resample_sampler_rules.pytests/unit/test_resample_to_reference.pytests/unit/test_resample_transform.pytests/unit/test_sampling.pytests/unit/test_streamed_read_dispatcher.pytests/unit/test_streamed_write_dispatcher.pytests/unit/test_transform.pytests/unit/test_transform_bound.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)
- docs/source/reference/api/extension-points.md
🚧 Files skipped from review as they are similar to previous changes (35)
- tests/unit/test_packaging.py
- AGENTS.md
- examples/Transform/Transform.yml
- konfai/data/data_manager.py
- tests/integration/test_transform_doc_examples.py
- tests/unit/test_transform.py
- tests/unit/conftest.py
- examples/Transform/README.md
- docs/source/reference/components/transforms.md
- tests/unit/test_resample_transform.py
- docs/source/reference/environment.md
- konfai/data/case_reduction.py
- tests/unit/test_sampling.py
- tests/unit/test_write_pyramid_and_field_bound.py
- tests/unit/test_transformer_workflow.py
- tests/unit/test_case_expansion.py
- tests/integration/test_konfai_streamed_prediction.py
- konfai/data/augmentation.py
- tests/unit/test_geometry.py
- tests/unit/test_streamed_read_dispatcher.py
- tests/unit/test_resample.py
- tests/unit/test_transform_locality_contract.py
- tests/unit/test_warp.py
- tests/unit/test_streamed_write_dispatcher.py
- konfai/utils/runtime.py
- konfai/utils/dataset.py
- CHANGELOG.md
- konfai/data/sampling.py
- docs/source/concepts/streaming.md
- docs/source/config_guide/transform.md
- tests/unit/test_transform_bound.py
- konfai/predictor.py
- konfai/data/geometry.py
- tests/unit/test_resample_to_reference.py
- konfai/data/patching.py
4f90108 to
5ca4d28
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
konfai/utils/ITK.py (1)
250-274: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winDowncast the top-level transform too, not only composite members.
Line 256 downcasts each composite member. The top-level transform is used as received.
sitk.ReadTransformreturns a downcast transform only from SimpleITK 2.2.0 onward; the v2.2.0 notes state that "The Python ReadTransform method now returns a downcasted transform."Transform.Downcastitself exists since 2.0.0: the v2.0.0 notes list "Add Python Transform.Downcast method."On SimpleITK 2.0 or 2.1 a stored BSpline or displacement field read from disk therefore arrives as the generic wrapper. It fails the
isinstancechecks at lines 260 and 268, failsIsLinear()at line 273, and is refused at line 275 with a message naming the generic type.Downcast()is a no-op on an already concrete transform, so applying it once at entry removes the dependency on the reader's behavior.This also relates to the earlier request to pin a SimpleITK minimum version. Confirm the declared constraint in
pyproject.toml.🐛 Proposed fix
_require_simpleitk() + transform = transform.Downcast() if isinstance(transform, sitk.CompositeTransform):#!/bin/bash # Read the declared SimpleITK constraint and check where transforms are read from disk. set -uo pipefail rg -nP -C 2 -i 'simpleitk' pyproject.toml 2>/dev/null rg -nP -C 4 'ReadTransform' konfai --type=py🤖 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 250 - 274, Update decode_transform_stages to call Downcast() on the top-level transform immediately after _require_simpleitk(), before the CompositeTransform and concrete-type checks. Preserve the existing member downcasting and dispatch behavior, and confirm pyproject.toml declares a SimpleITK version constraint compatible with this use.
🧹 Nitpick comments (3)
konfai/data/sampling.py (1)
157-207: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winConsider caching the device copy of
stage.values.Line 159 uploads the whole displacement or coefficient grid to the device on every call. The streamed path calls
_displacement_atonce per slab, so a case pays the same copy for every slab of the volume. For a dense field the copy isrank× the field size in float64.A small cache keyed on the stage plus device would remove the repeat without changing any arithmetic. The stages are immutable dataclasses, so a
WeakKeyDictionaryin the caller or a memo on the stage would suffice.🤖 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/sampling.py` around lines 157 - 207, Cache the device-specific tensor created from stage.values in the caller of this sampling logic, keyed by the immutable stage and target device, and reuse it across _displacement_at slab calls. Replace the per-call torch.tensor conversion with the cached tensor while preserving its dtype, device, and existing arithmetic.konfai/data/patching.py (2)
259-275: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider dropping the
namedefault on_RemapPull.The docstring states that a pull that cannot say which case it serves builds a window from another case's map, and that a short window returns the fill instead of raising. The only construction site (Line 1826) passes
self.name, so the""default adds no value and keeps that silent failure reachable after a future refactor. Make the field required.♻️ Proposed change
remap: Callable[[str, tuple[slice, ...], list[int], Attribute], list[slice]] shape: list[int] attribute: Attribute - name: str = "" + name: strNote: check the dataclass field order of
_RemapPullbefore applying, because a required field cannot follow a defaulted one.🤖 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/patching.py` around lines 259 - 275, Make the name field of _RemapPull required by removing its empty-string default, and preserve valid dataclass field ordering by placing it before any defaulted fields. Keep the existing construction path passing self.name unchanged.
2163-2163: 🗄️ Data Integrity & Integration | 🔵 Trivial | 💤 Low valueDocument the
prefer_wholebehavior inmaterialize.
Transformerintentionally passesprefer_whole=Truewithallow_fallback=Falsefor plannedLOADentries. State in the docstring thatprefer_whole=Trueselects whole-volume assembly and bypassesallow_fallback.🤖 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/patching.py` at line 2163, Update the materialize docstring to document that prefer_whole=True selects whole-volume assembly and bypasses allow_fallback, including the intentional Transformer usage for planned LOAD entries.
🤖 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/data/patching.py`:
- Around line 1746-1753: Update the docstring for the stage-planning function
around the `GLOBAL_STAT`/`REGRID` refusal description: remove the obsolete
`REGRID` checks for missing `Spacing` or non-`Resample` stages, and state that
refusal is determined by the stage’s `patch_locality` declaration. Leave the
surrounding stage-chain and shape-folding behavior unchanged.
In `@konfai/data/sampling.py`:
- Around line 432-447: Update the nearest branch in the sampling function to
gather values directly from source rather than the sampling_dtype-upcast work
tensor. Preserve the existing index selection and output shape, and cast only
the fill value or masked-fill operation as needed so integer labels retain their
original precision and dtype.
In `@pyproject.toml`:
- Around line 55-59: Lower the SimpleITK minimum version from >=2.1 to >=2.0.0
in the itk and imaging extras, and apply the same >=2.0.0 constraint in the Pixi
development environment and development dependencies. Keep all other package
constraints unchanged.
---
Duplicate comments:
In `@konfai/utils/ITK.py`:
- Around line 250-274: Update decode_transform_stages to call Downcast() on the
top-level transform immediately after _require_simpleitk(), before the
CompositeTransform and concrete-type checks. Preserve the existing member
downcasting and dispatch behavior, and confirm pyproject.toml declares a
SimpleITK version constraint compatible with this use.
---
Nitpick comments:
In `@konfai/data/patching.py`:
- Around line 259-275: Make the name field of _RemapPull required by removing
its empty-string default, and preserve valid dataclass field ordering by placing
it before any defaulted fields. Keep the existing construction path passing
self.name unchanged.
- Line 2163: Update the materialize docstring to document that prefer_whole=True
selects whole-volume assembly and bypasses allow_fallback, including the
intentional Transformer usage for planned LOAD entries.
In `@konfai/data/sampling.py`:
- Around line 157-207: Cache the device-specific tensor created from
stage.values in the caller of this sampling logic, keyed by the immutable stage
and target device, and reuse it across _displacement_at slab calls. Replace the
per-call torch.tensor conversion with the cached tensor while preserving its
dtype, device, and existing arithmetic.
🪄 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: ab1c0a13-1a2c-4433-816f-cb2ac5316831
⛔ Files ignored due to path filters (1)
pixi.lockis excluded by!**/*.lock
📒 Files selected for processing (43)
AGENTS.mdCHANGELOG.mddocs/source/concepts/streaming.mddocs/source/config_guide/transform.mddocs/source/reference/api/extension-points.mddocs/source/reference/components/transforms.mddocs/source/reference/environment.mddocs/source/usage/large-images.mdexamples/Transform/README.mdexamples/Transform/Transform.ymlkonfai-mcp/konfai_mcp/catalog.pykonfai/data/augmentation.pykonfai/data/case_reduction.pykonfai/data/data_manager.pykonfai/data/geometry.pykonfai/data/patching.pykonfai/data/sampling.pykonfai/data/transform.pykonfai/predictor.pykonfai/transformer.pykonfai/utils/ITK.pykonfai/utils/dataset.pykonfai/utils/runtime.pypyproject.tomltests/integration/test_konfai_streamed_prediction.pytests/integration/test_transform_doc_examples.pytests/unit/conftest.pytests/unit/test_case_expansion.pytests/unit/test_geometry.pytests/unit/test_packaging.pytests/unit/test_resample.pytests/unit/test_resample_sampler_rules.pytests/unit/test_resample_to_reference.pytests/unit/test_resample_transform.pytests/unit/test_sampling.pytests/unit/test_streamed_read_dispatcher.pytests/unit/test_streamed_write_dispatcher.pytests/unit/test_transform.pytests/unit/test_transform_bound.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)
- docs/source/reference/api/extension-points.md
🚧 Files skipped from review as they are similar to previous changes (37)
- tests/unit/test_packaging.py
- tests/integration/test_konfai_streamed_prediction.py
- tests/integration/test_transform_doc_examples.py
- konfai-mcp/konfai_mcp/catalog.py
- konfai/utils/runtime.py
- tests/unit/test_transform.py
- examples/Transform/README.md
- examples/Transform/Transform.yml
- docs/source/reference/environment.md
- konfai/data/case_reduction.py
- tests/unit/test_case_expansion.py
- AGENTS.md
- konfai/data/augmentation.py
- tests/unit/test_write_pyramid_and_field_bound.py
- tests/unit/conftest.py
- konfai/data/data_manager.py
- tests/unit/test_transformer_workflow.py
- tests/unit/test_resample_transform.py
- tests/unit/test_resample_sampler_rules.py
- tests/unit/test_warp.py
- CHANGELOG.md
- tests/unit/test_sampling.py
- tests/unit/test_streamed_write_dispatcher.py
- tests/unit/test_transform_bound.py
- docs/source/concepts/streaming.md
- docs/source/reference/components/transforms.md
- konfai/data/geometry.py
- tests/unit/test_streamed_read_dispatcher.py
- tests/unit/test_transform_locality_contract.py
- konfai/utils/dataset.py
- tests/unit/test_geometry.py
- docs/source/config_guide/transform.md
- tests/unit/test_resample.py
- konfai/predictor.py
- konfai/transformer.py
- tests/unit/test_resample_to_reference.py
- konfai/data/transform.py
Three findings, each reproduced by execution before fixing:
- The end-of-run totals reduce followed no backend: a CPU tensor under
NCCL, so every --gpu TRANSFORM run died on bookkeeping after all its
cases were written. The tensor now follows the backend.
- apply_to_data_transform (with compose_transform and the
_open_transform cluster) is restored to utils/ITK.py: it was deleted
as orphaned, and apps/impact_reg — released in lockstep — imports it
at module level to warp landmarks. Its tests come back with it.
- Clip('min'/'max') and Statistics trusted any 'Min'/'Max'/'Mean'/'Std'
key, including the bookkeeping an upstream Normalize pushes for its
own inverse: [Normalize, Statistics] recorded the pre-normalization
extrema beside a post-normalization mean. The read baselines and the
reduction's own seeding pass now carry a chain-scoped
StatisticsSeeded marker — set before the persistence snapshot, so it
never outlives the read — and the two stages trust a statistic only
under it.
And a fourth, smaller: an Expand copy's fold state now adopts Crop's
box, as the case-level folds do, instead of re-reading the volume once
per later fold.
…ps have register/eval/uncertainty staged under tempfile.gettempdir() with no way to say otherwise, so a caller whose TMPDIR is a tmpfs paid volume-sized intermediates in RAM and had no recourse but to override TMPDIR for the whole subprocess -- which also moves what TMPDIR legitimately owns (torch's DataLoader worker sockets, whose AF_UNIX addresses cap at ~108 bytes). --tmp-dir is what every other KonfAI app CLI already exposes through build_app_cli; impact-reg is the only orchestrator that builds the konfai-apps command line by hand (one subprocess per preset, because konfai keeps process-global state) and that hand- built list had dropped this one flag of the ten it forwards. Forwarding it to konfai-apps also removes a full-size write: given a caller-owned workspace, konfai-apps writes straight into -o instead of staging ./Predictions and copying it in (see _stage_result_dir / _collect_result). The moved image and the displacement field are now written once per preset rather than twice. Default unchanged: with no --tmp-dir the commands stage where they always did.
…mmand --tmp-dir reached konfai-apps infer but not the two other nested invocations, so a caller who placed the staging deliberately still had 'uncertainty' stage its Uncertainties under the system TMPDIR, and the three in-process 'evaluate' calls auto-create a workspace of their own. Both are the traffic the option exists to move, so the option only half worked. uncertainty now passes the private directory _work_dir already made inside the caller's tmp_dir, and evaluate forwards the same one through the tmp_dir the konfai-apps API accepts. Tests pin the forwarding for each nested command.
… moved image A registration preset's output IS the displacement field; the moved image is that field applied to the moving. Requiring both made every preset carry a second output this layer can produce itself -- and for a tiled preset, blend it across every patch seam only for a caller that reads the field to discard it. _find_output gains required=False, so _infer_preset returns Moved or None, and register derives it when the preset left it out. Deriving it needs the moving image back, which is where the ensemble path was already wrong: it re-read with sitk.ReadImage, which cannot open a directory, so an ensemble over OME-Zarr inputs failed there and nowhere else -- the same regression 1.6.0 fixed for the single-preset path by never re-reading at all. Both paths now go through _read_image / _write_image, a dispatch mirroring konfai's read_displacement_field and the existing _write_displacement_field, so the format in is the format out and the resample never decides it. The resample itself goes through SimpleITK on the field's own grid, for the reason konfai's ResampleTransform gives: the stored displacement is in world (x, y, z) units and adding it onto a (z, y, x) voxel grid transposes the axes.
…t branch The dispatch this replaces knew exactly two formats -- an ITK file or an OME-Zarr store -- because those are the two whoever wrote it had in front of them. konfai already has the layer that knows them all: Dataset covers h5, DICOM, OME-Zarr and every ITK extension through SitkFile, which probes them itself, and re-detects a directory store from disk whatever the format token said. _dataset_entry addresses a bare path as a Dataset entry -- root is the parent, the stem is the entry, the case is empty -- so _read_image and _write_image are now two lines each and inherit every format konfai supports, in and out. evaluate's four input reads went through sitk.ReadImage as well, so evaluating an OME-Zarr pair failed the same way the ensemble path did. They go through the same reader now; no direct sitk.ReadImage of a caller-supplied input is left.
_infer_preset looked for a Moved as well as a DVF and reused it when it was there. That made the moved image half a contract: presets had to write it, the orchestrator had to branch on it, and a tiled preset blended a full-size one across every patch seam for a caller that then read only the field. A registration app produces a displacement field on the fixed grid, in whatever format it declares. That is the whole contract. Everything computable from it -- the moved image above all -- is this layer's job, so the moved is now always derived and the branch is gone. Measured while checking the output layout: a preset fed .mha inputs writes its field as .ome.zarr when that is what it declares. So the input's form says nothing about the output's; the derived moved follows the FIELD, which is the only thing the preset committed to. The docstring said otherwise.
…nts given
register numbered its cases from the command line -- one P{index} per (fixed,
moving) pair of paths -- while konfai-apps numbers them from the EXPANDED units:
each -i is a group, a file or a store is one unit, and a plain directory is
walked so every volume inside becomes its own. The two agree only while each
group holds exactly one file.
Hand it a directory and they diverge: konfai-apps registered every volume in it,
_find_output took sorted(rglob(...))[0], and every case after the first was
written to disk and dropped without a word. Verified on a real two-case run
before the fix -- P001 computed, then discarded -- and after it: two directories
in, two complete cases out, each with its field, its moved image and its
transform.
So the presets now run once over the whole cohort rather than once per pair (one
model load instead of N), _find_outputs returns a field per case, and register
iterates over the cases konfai-apps named rather than renumbering its own. The
moving volume each case is derived from comes from _list_input_units, the same
expansion konfai-apps uses, so the two orders cannot drift apart.
evaluate counted its cases the way register used to -- max() over the argument lists -- so a directory of volumes evaluated its first entry and nothing else, the same silent truncation just fixed one function above. Every group now goes through _units, the shared wrapper over konfai-apps' _list_input_units, so the two commands agree on what a case is. Transforms and landmark files expand too: .h5, .fcsv and .itk.txt are all supported extensions, so a directory of transforms pairs with a directory of volumes.
… needs Reading went through Dataset already, but by pretending a bare path was one: the parent directory taken for a root, the stem for a group, the case left empty. That is not a dataset, and it showed -- detection probes a case's entries, so on a flat layout it descended INTO the store and found nothing, forcing a format token to be guessed from the suffix, which is the dispatch Dataset was meant to replace. For a single-store backend like h5 it was not merely awkward but wrong: the file is the dataset, not an entry in one. A dataset is a root of cases holding groups, and it is built. So the path is linked into that layout first -- one case, one group, exactly as konfai-apps stages its own inputs -- and read with NO format named. konfai has a case to probe and detects the backend itself. The write side needed no such thing: <output>/<case>/<group> already IS that layout, so it goes through Dataset.write with the format named, which writing does require since nothing is on disk yet to detect. Verified: the OME-Zarr and ITK reader tests pass with no token, and a real two-case run over two directories still yields both cases complete.
The moved image and Transform.h5 are both derived FROM the displacement field -- a full-size resample of the moving and a full-size rewrite of the same voxels. Worth it for a caller that wants a registration to look at; waste for one that composes the field with another and derives its own moved from the total. The ExaSPIM tiled refinement is the second kind: it reads the field out of P000, composes it with the global pass and resamples through the COMPOSED transform, so the moved derived from the tiled residual alone is meaningless to it and the transform is a copy of a field it already holds. Both were written and deleted. --fields-only stops after the field. Nothing changes without it.
Resample's reference accepts '{case}': each case adopts the grid of its
own entry in reference_group -- the registration idiom, where a moved
image belongs on its field's grid. The reference-grid memo is per
resolved entry, so a literal reference stays one lookup for the cohort.
write_stream_cache_attribute gains the case name, and the landing folds
(patching, transformer) and streamed writes (predictor) pass it: a
per-case target grid resolves during planning and streaming, not only
in __call__.
Std folds cases element-wise with Welford running moments: incremental and voxel-local, so the peak is two accumulators plus the member being read; unbiased to match torch.std; zeros for a single case. Magnitude is the vector norm over the CHANNEL axis, POINTWISE, so the magnitude of a displacement field read as a case streams slab by slab. Norm keeps the trailing-axis stacked layout.
konfai.api, re-exported lazily at top level: transform, plan_transform,
evaluate, predict, train. A chain is a list of live stage objects or
the equivalent mapping; the tree goes through the binder unchanged, and
the resolved YAML still lands in the workspace as the run's record.
Two halves make the object spelling possible without a second grammar:
configure_workflow_environment materializes a {root: {...}} dict into a
config file, so every workflow entry point takes the tree as well as a
path; and record_given_arguments (applied through __init_subclass__ on
Transform, DataAugmentation and Criterion) stores on each instance the
kwargs the caller passed -- the binder's mirror. The outermost
constructor records; a delegating super().__init__ keeps the caller's
spelling; *args marks the instance unrecordable and it is refused by
name.
The contract differs from the CLI: a designed refusal raises
KonfAIError instead of exiting, results come back structured (the
outputs.json destinations, the parsed Metric_*.json), the KONFAI_*
environment is restored around every call, and one workflow runs at a
time per process -- a second concurrent call is refused with the
remedy. tests/unit/test_api.py pins the recording, the serialization,
byte-identity between the object and tree spellings of one run, and
the contract.
The orchestrator stages symlinked cohorts and calls konfai.api: it
holds no volume in RAM and resamples nothing by hand.
- The moved images are ONE streamed run for the whole cohort: Resample
adopts each case's own DVF grid (reference '{case}'), reads the field
as the map (field_group), and Write lands Moved beside the DVF.
--max-displacement bounds the window a streamed slab reads; 'auto'
reads the bound a field recorded (OME-Zarr fields carry one) and
falls back to whole-volume with the reason in the plan.
- The ensemble average is Reduce(Mean) over members-as-cases, per case:
grid strict verifies the shared-grid claim, the fold is incremental,
and the written store keeps its displacement-field declaration.
- uncertainty is Magnitude -> Reduce(Std) -> Write: no stacked volume,
no preset app; the preset argument stays accepted and unused.
- evaluate's image and seg warps are the same Resample with the stored
transform staged as a group the engine decodes; landmarks still open
the transform in-process, a few points.
- Transform.h5 stays SimpleITK: the format itself carries the whole
field, which is why it is gated behind fields_only.
The CLI prints a KonfAIError's message and remedy and exits 1.
The TRANSFORM plan answers LOAD -- the case fits the budget and streaming would reread the source past its worth -- and the tool description enumerated every verdict but that one. The generated tool reference follows.
A field with no declared or recorded bound was a whole-volume answer: the plan could not size what a region must read without reading the field. It streams now. The field window a region samples is its own box, read for sampling regardless -- and the sup of those very values bounds every interpolated displacement in the region (a convex combination cannot exceed the lattice values it blends). The window is memoized so sizing and sampling share one read, and the halo is per region and per component: a quiet slab pays a quiet halo. The plan stays headers-only: the run walks a measured pull (_ReadStagePlan.run_pull) while the estimator prices the declared one -- as if the field were zero when nothing bounds it, and the plan prints that note. A declared or recorded bound keeps the declared windows, whose streamed result is pinned bit-identical to the whole path, and stays checked against every region read; measuring is the route for the field that could not stream at all before. An unreadable entry in the field group keeps its whole-volume answer -- it fails both routes at run.
The parameter asked the user for a number the run now measures better: each region's source window is sized from the field values read for sampling. What remains needs no declaration -- the bound a STORE recorded at write time (KonfAI's OME-Zarr fields carry one) is read from headers, prices the plan exactly, and is checked per component against every region read: metadata that contradicts its data raises instead of sampling zeros. Removed from Resample, Warp and ResampleToReference; the impact_reg CLI loses --max-displacement. The streamed-equals-whole pin for a warped reference moves from bit-equality to the accepted band for maps that do not factorise: its exactness came from a declared bound large enough to make every window the whole source, and the measured windows are the region's own. The bit-exact claims stay on the separable path, which is window-independent by construction.
ResampleToResolution, ResampleToShape, ResampleToReference,
ResampleTransform and Warp are removed: five ways to reference the one
stage, kept as forwarding shells. A config names Resample and says
which grid (spacing, shape, reference) and which map (field,
transforms); two Resample stages in one chain spell the second
module-qualified (konfai.data.transform:Resample), which the strict
grammar and the loader already resolve.
A blank reference is refused at construction. The locality-contract
registry folds the family's cases under one Resample entry; the docs
lose the deprecated-spelling rows and notes.
BREAKING CHANGE: published configs using the old names must rename the
key -- ResampleToResolution: {spacing: [...]} becomes
Resample: {spacing: [...]}. The HF bundle configs move with the konfai
version they pin.
…able A directory dataset's backend is detected from its first case, and one store entry flips the whole root to the store backend: an OME-Zarr moving staged beside the .mha field every published preset declares made the field unreadable at the first register. Each group now stages into a root of its own -- homogeneous, so each keeps its backend -- and the run reads its roots side by side. Also: re-staging a case replaces its link instead of raising, and _the_output names the stale other-form file it found and the remedy.
sitk.WriteTransform needs the whole field resident, converted to float64 -- the run's peak, per case, once everything else streams. The constraint is the function, not the format: an ITK transform file is three HDF5 datasets (type; size/origin/spacing/direction; the field buffer, component fastest), and HDF5 writes by regions. The file is now written through h5py with the parameters streamed from the store, so the peak is one slab in float64; without h5py the sitk path serves, whole. The test pins read-back equality with sitk's own writer -- same type, fixed parameters and parameters, exactly, from either field form.
The changelog told readers 'the old names still work and are thin argument translations'. That was true of the commit that made them forwarding shells, not of the one that removed them -- and it is the section the CI publishes as the release note, so it would have sent people migrating after the fact, onto something broken. It now carries the argument mapping. The visual gallery still imported ResampleToShape and ResampleToResolution.
The ':itktransform' format writes a displacement field as the ITK transform file any ITK consumer loads -- three HDF5 datasets (the type, ASCII as ITK reads it; size/origin/spacing/direction; the field buffer, component fastest) -- and fills the parameters region by region: sitk.WriteTransform needs the whole field resident in float64 where the file itself writes by slabs. Whole and streamed writes are pinned identical to sitk's own writer through ITK's reader; an aborted stream leaves no entry under the final name. The read side hands back what Dataset.read_transform decodes -- a displacement entry with its field and marker, any other stored transform as parameter rows -- so a staged .h5/.tfm resolves through the same Dataset surface as every other entry. Without h5py the sitk path serves, whole.
The register run gains a second chain, DVF -> Transform: a plain Write to the ':itktransform' backend, which fills the file region by region. One plan covers the moved image and the transform, resume covers both, and the orchestrator's last hand-rolled writer is gone. Staged .h5 and .tfm transforms route to the same backend, so evaluate's Reg group resolves through Dataset like every other entry.
… image Each engine resampled the moving onto the fixed grid and returned it alongside the field, the two concatenated on the channel axis for a pair of ChannelSelect modules to split again. Since a preset declares only its DisplacementField, konfai writes none of it -- but all three engines still computed it, a full-grid sitk.Resample per case, thrown away. The orchestrator derives the moved image from the field anyway, streamed, so the common path resampled twice: once in the engine for nothing, once in impact-reg for real. elastix, fireants and convexadam now return the field alone, the modules stop concatenating, and MovedImage is gone from the three models.
MaxDisplacement is gone: the OME-Zarr writer records nothing, the streamed field write accumulates nothing, and the reader scans nothing -- the run sizes every region's pull from the field values it reads for sampling, and the plan prices those reads as a zero field and says so in its note. What remains of the plan-time scan is the one thing it still owes: every field entry's header must open, or the group answers whole-volume before a case is chosen. BREAKING CHANGE: DISPLACEMENT_BOUND_ATTRIBUTE and displacement_bound are removed from konfai.utils.ome_zarr; stores carrying the attribute simply have it ignored.
The parameters are HDF5, so a span of leading-axis rows is one contiguous span of the dataset: a region of a displacement entry decodes the rows it maps to and nothing else, and bounded_region_reads says so to the plan. A foreign or non-displacement file keeps the whole-read path.
The region-writable SimpleITK set becomes the region-readable one: uncompressed .nii is a fixed 348-byte header plus a flat raw block, exactly the property the .mha stream memmaps -- and NIfTI's vector dimension is its slowest, so channel-first slabs land without a transpose. The one convention the stream owns is the RAS sform (the pipeline speaks LPS; the affine's first two rows negate on the way out), pinned against sitk's own writer on an oblique grid. nii joins the stream test matrix, so concurrency, abort and invisibility hold for it as for the others.
Drop the hand-written Unreleased section and the entries the branch had appended into the released v1.8.0 section: v1.8.0 shipped without this branch, and the file's own preamble says each section is drafted by commitizen at tag time, then edited. The next tag documents this line.
…rived A preset declares one output: its transform, under whatever name and in whatever form its consumer reads. register discovers that group instead of assuming DVF, refuses an ensemble whose members disagree on it, and no longer derives Transform.h5 -- a preset that serves Slicer writes the ITK file itself, so there is no second copy of the same field under another name. Only the moved image is still derived.
…onal `:itktransform` writes `<group>.h5` -- nothing on disk is ever named `.itktransform`. Validating a dataset spec against the extension list therefore rejected the very format the write side had just produced: a run could write a transform it could not read back. Tokens that name a backend rather than a suffix now live in SUPPORTED_BACKEND_FORMATS, and SUPPORTED_FORMATS -- the union -- is what a `path[:flag]:format` spec is checked against, down to `split_path_spec`'s parameter. SUPPORTED_EXTENSIONS keeps its own job: the suffixes probed beside a case and matched against an input path. The backend also stops degrading in silence. Without h5py it fell back to holding the field whole in float64 and writing it through sitk, so peak memory turned on whether an optional import had succeeded -- and neither path was covered, the guarantee test being skipped when h5py is absent. It now raises and names the extra, which is what the `h5` backend has always done in effect (an AttributeError on None; it says so properly now too). impact-reg declares h5py: every preset writes its transform through this backend.
…kend get_infos opened every entry as HDF5; a legacy text .tfm is not one, and now takes the whole-decode path. The leading-axis step was dropped by the span read -- rows start..stop came back whole -- and now subsamples the reshaped block; a reversed axis falls back to the whole read. Both pinned by tests, with a foreign text file among them.
5ca4d28 to
4337e9c
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
vboussot
changed the title
There was a problem hiding this comment.
Actionable comments posted: 6
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
konfai/data/patching.py (1)
1477-1499: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winPreserve
state_initattribute updates in theExpandpath
Elastix._state_initaddsSpacing,Origin, andDirection, but_adopt_case_factscopies onlybox. TheExpandpath therefore drops these keys, while_draw_augmentation_listspreserves them. Propagate all required attribute updates, or return them explicitly fromstate_init.🤖 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/patching.py` around lines 1477 - 1499, Update the Expand handling around stage.state_init and _adopt_case_facts so all attribute updates produced by state_init, including Spacing, Origin, and Direction, are preserved in each copy’s attributes rather than only box. Reuse the existing attribute propagation approach used by _draw_augmentation_lists, or extend _adopt_case_facts to copy the complete required state-init attributes.
🟡 Minor comments (18)
docs/source/usage/large-images.md-195-205 (1)
195-205: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAdd coverage for the documented guarantees or narrow the wording.
The stored-transform test supports the
1e-5range-relative tolerance. The field and integration tests use different tolerances. No test covers axis-aligned streamed-versus-whole identity or near-tied logits afterargmax.🤖 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 `@docs/source/usage/large-images.md` around lines 195 - 205, Update the documentation paragraph around the streamed resampling guarantees to match existing test coverage, or add tests covering each stated guarantee: bit-identical axis-aligned streamed versus whole-volume output and near-tied blended-logit argmax behavior. Ensure tolerance claims consistently reflect the field, stored-transform, and integration tests, especially the documented 1e-5 range-relative value.docs/source/usage/python-workflows.md-44-45 (1)
44-45: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
plan_transformdoes not take the same arguments astransform.Line 44 states that
konfai.plan_transform(...)"takes the same arguments". Inkonfai/api.py,transformacceptsgpuandquiet;plan_transformaccepts neither. A reader who copies atransformcall and swaps the function name gets aTypeErrorongpu=[0].State that it takes the same planning arguments, and note that the execution-only arguments do not apply.
🤖 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 `@docs/source/usage/python-workflows.md` around lines 44 - 45, Update the `plan_transform` documentation to say it accepts the same planning arguments as `transform`, while explicitly noting that execution-only arguments such as `gpu` and `quiet` are not supported. Preserve the explanation that it returns a `TransformPlan` without executing anything.konfai/api.py-227-230 (1)
227-230: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
dataset_optionssilently overwrites the dedicated arguments in both workflow builders. Each site builds adataset_treefrom its own parameters and then merges the free-formdataset_optionsmapping over it, so a colliding key replaces an explicit argument with no message. The shared root cause is the merge order; one collision check applied at both sites fixes it.
konfai/api.py#L227-L230: in_transform_tree, refusedataset_optionskeys that collide withdataset_filenames,groups_srcormemory_budgetbefore the merge, so a straygroups_srcentry cannot discard thechainsargument.konfai/api.py#L357-L358: inevaluate, apply the same refusal againstdataset_filenamesandgroups_src, so the groups derived frommetricscannot be replaced silently.🤖 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/api.py` around lines 227 - 230, The dataset_options merge can overwrite dedicated dataset arguments in both workflow builders. In konfai/api.py lines 227-230 within _transform_tree, reject or raise before merging any dataset_options key colliding with dataset_filenames, groups_src, or memory_budget; in konfai/api.py lines 357-358 within evaluate, apply the same collision refusal for dataset_filenames and groups_src. Preserve the existing merges for non-conflicting options.apps/impact_reg/README.md-87-87 (1)
87-87: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winName the CLI flag the CLI actually accepts.
Line 87 documents
--keep_dvf._dispatchinapps/impact_reg/impact_reg_konfai/cli.pypasseskeep_dvf=args.uncertainty, so the parsed CLI option is--uncertainty;keep_dvfis the Python keyword only. A reader who copies--keep_dvfgets an argparse error.The
registerarguments table at lines 116-124 also omits both this flag and--fields_only, which_dispatchforwards asfields_only. Add them so the table matches the parser.📝 Confirm the real option strings before editing
#!/bin/bash # Description: List the register sub-command options the CLI defines. set -euo pipefail rg -n -C2 'add_argument\(\s*"--(uncertainty|keep_dvf|fields_only|tmp-dir|tmp_dir)' apps/impact_reg/impact_reg_konfai/cli.py🤖 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/README.md` at line 87, Update the register documentation to name the accepted --uncertainty flag instead of --keep_dvf, and add both --uncertainty and --fields_only to the register arguments table. Keep the descriptions aligned with how _dispatch forwards these parsed options as keep_dvf and fields_only.apps/impact_reg/impact_reg_konfai/impact_reg.py-614-616 (1)
614-616: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winA short
transformslist silently evaluates the remaining cases with identity.Line 614 derives
n_casesfrom the fixed inputs. Line 616 falls back toNonewheneverindex >= len(transforms). If the caller passes two transforms for three cases, case index 2 is evaluated as if it were already registered, and the metrics are reported without any indication.Every other group in this method pairs by position and
registerrefuses a mask-count mismatch (lines 414-418). Apply the same rule here.🛡️ Require zero transforms or one per case
n_cases = max(len(fixed_images), len(gt_fixed_seg), len(gt_fixed_fid)) + if transforms and len(transforms) != n_cases: + raise RuntimeError( + f"the transforms expand to {len(transforms)} unit(s) for {n_cases} case(s);" + " transforms pair with cases by position, so give one per case or none at all." + ) for index in range(n_cases):🤖 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 614 - 616, Update the case-count and transform selection logic around n_cases so transforms must contain either zero entries or exactly one entry per case; reject any other nonzero length before evaluation, consistent with register’s mask-count validation. Preserve the existing identity behavior only when transforms is empty, and ensure every evaluated case uses its positional transform when transforms are provided.apps/impact_reg/tests/unit/test_displacement_field_io.py-223-235 (1)
223-235: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winGate this test on
_zarr_v3_available().
_write_storecalls_write_displacement_field, which callswrite_ome_zarr(..., displacement_field=True). That path runs_require_zarr_v3_for_rfc5(), so it raises when the installed zarr is 2.x._zarr_v3_availabledocuments exactly this: RFC-5 axis types need a zarr v3 store, and Python 3.10 gets zarr 2.x.This test takes no
suffixparameter, so it does not inherit whatever gate the parametrized tests use. On a zarr 2.x environment it fails instead of skipping.apps/impact_reg/tests/unit/test_orchestration.pyline 222 applies the gate for the same reason.💚 Skip when the store cannot be written
def test_ensemble_field_written_by_the_orchestrator_is_a_declared_field(tmp_path: Path) -> None: """The averaged DVF is folded by Reduce(Mean) and written through konfai's Write, in the members' form — and must stay a DECLARED field: ``read_displacement_field`` refuses an undeclared 3-channel store, so dropping the declaration would break ``evaluate`` right after a successful register. The values are the voxel-wise mean, on the members' geometry.""" + if not _zarr_v3_available(): + pytest.skip("writing a declared displacement field needs zarr 3") members = []🤖 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_displacement_field_io.py` around lines 223 - 235, Gate test_ensemble_field_written_by_the_orchestrator_is_a_declared_field on _zarr_v3_available() before creating stores or invoking _ensemble_mean. Use the same skip mechanism and reason as the comparable gate in test_orchestration.py, preserving the test’s existing assertions and behavior when zarr v3 is available.apps/impact_reg/impact_reg_konfai/impact_reg.py-743-751 (1)
743-751: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winCount the ensemble members after expansion, not before.
Line 743 checks
len(dvfs) < 2on the raw argument list. Line 749 then expands those paths into units, where one directory becomes many members. A caller who passes a single directory holding five fields is refused with a message about needing two fields, although five are present. The reverse also holds: two arguments that expand to one unit each pass the check and then produce a single-memberStd.Expand first, then validate the member count.
🛠️ Validate the expanded members
del preset - if len(dvfs) < 2: - raise ValueError("Uncertainty needs at least two ensemble displacement fields.") work = _work_dir(tmp_dir, "impact_reg_unc_") try: from konfai.data.transform import Magnitude, Reduce, Write members = _units(list(dvfs)) + if len(members) < 2: + raise ValueError( + f"Uncertainty needs at least two ensemble displacement fields; the given" + f" path(s) expand to {len(members)}." + )🤖 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 743 - 751, Move the minimum-member validation in the uncertainty setup around `_units` so it checks the expanded `members` collection rather than raw `dvfs`. Expand `dvfs` first, then raise the existing ValueError when `len(members) < 2`, before using `members[0]` or building the stage group.konfai/metric/measure.py-78-83 (1)
78-83: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAdd configuration-binding tests for criterion constructors.
Criterion.__init_subclass__now changes how criterion instances serialize back into configuration. Add coverage intests/unit/test_config.pyfor explicit positional arguments, keyword arguments, defaults, and a subclass that delegates tosuper().__init__.As per coding guidelines, “When changing configuration binding, update
tests/unit/test_config.py; add tests for new union-typed configuration keys.”🤖 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/metric/measure.py` around lines 78 - 83, Add configuration-binding coverage in tests/unit/test_config.py for Criterion constructors, using the relevant Criterion subclasses and their __init_subclass__ behavior. Verify serialization preserves explicit positional arguments, keyword arguments, and default values, and include a subclass whose constructor delegates to super().__init__.Source: Coding guidelines
docs/source/config_guide/transform.md-279-303 (1)
279-303: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winRemove the stale direction refusal further down the page.
This section documents the unified
Resample. The refusal list at Lines 428-430 still states that a reference whoseDirectiondiffers from the case's is refused, and tells the reader to runCanonicalfirst. The new implementation resamples through the rotation instead, andtests/unit/test_resample_to_reference.py::test_a_differing_direction_is_resampled_and_not_refusedpins that behavior. The refusal list at Lines 477-483 already omits the clause, so the page contradicts itself.Delete that bullet so the two lists agree.
🤖 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 `@docs/source/config_guide/transform.md` around lines 279 - 303, Remove the stale refusal-list bullet that says a reference with a differing Direction is refused or requires Canonical first. Update the refusal list for the unified Resample documentation so it matches the implemented rotation-aware behavior and the later list that already omits this restriction.tests/unit/test_resample.py-33-33 (1)
33-33: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winThe only test in this file needs no SimpleITK, but the module gate skips it.
Line 33 skips the whole module when SimpleITK is missing.
test_an_axis_the_map_leaves_alone_is_left_aloneuses only NumPy and torch, so an environment without theitkextra loses that coverage for no reason. Thesitkname is reached only through_as_image, which no test calls.Move the gate to the tests that need the oracle, or drop it until one exists.
🧪 Proposed change
-sitk = pytest.importorskip("SimpleITK") +sitk = pytest.importorskip("SimpleITK", reason="the resample oracle is SimpleITK's own")Apply the marker per test instead:
`@pytest.mark.usefixtures`() # or a module-level fixture that calls importorskip def test_that_needs_the_oracle() -> None: sitk = pytest.importorskip("SimpleITK") ...🤖 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_resample.py` at line 33, Remove the module-level SimpleITK import gate from tests/unit/test_resample.py so test_an_axis_the_map_leaves_alone_is_left_alone runs with only its NumPy and torch dependencies; if future tests require SimpleITK, apply pytest.importorskip locally within those tests or their fixture instead.docs/source/reference/components/storage-backends.md-27-30 (1)
27-30: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winUpdate the
imagingextra documentation.konfai[imaging]also installsdask, andngff-zarrrequires version0.38or later. List all six dependencies in the tip.🤖 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 `@docs/source/reference/components/storage-backends.md` around lines 27 - 30, Update the imaging installation tip to list all six dependencies, adding dask and specifying that ngff-zarr requires version 0.38 or later while preserving the existing SimpleITK, h5py, pydicom, and zarr entries.konfai/utils/dataset.py-2136-2141 (1)
2136-2141: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winExclude staging leftovers from the entry listings.
data_to_fileandopen_data_streamstage into<case>/.<name>.<pid>.tmp.h5.Path.globmatches names that start with a dot, so a crashed writer's leftover appears inget_namesandget_groupas an entry named.<name>.<pid>.tmp.H5File.get_namesalready filters.tmpkeys for the same reason.♻️ Proposed fix
def get_names(self, group: str) -> list[str]: del group - return sorted({path.stem for pattern in ("*.h5", "*.tfm") for path in Path(self.filename).glob(pattern)}) + return self._entries() def get_group(self) -> list[str]: - return sorted({path.stem for pattern in ("*.h5", "*.tfm") for path in Path(self.filename).glob(pattern)}) + return self._entries() + + def _entries(self) -> list[str]: + """The published entries: a staging write (``.<name>.<pid>.tmp.h5``) is not one.""" + return sorted( + { + path.stem + for pattern in ("*.h5", "*.tfm") + for path in Path(self.filename).glob(pattern) + if not path.name.startswith(".") + } + )🤖 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 2136 - 2141, Update H5 file entry listing in get_names and get_group to exclude staging leftovers whose filenames match the hidden .<name>.<pid>.tmp.h5 pattern, while retaining valid .h5 and .tfm entries and existing sorting behavior.konfai/utils/dataset.py-2046-2048 (1)
2046-2048: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winReturn
Falsefor a missing entry instead of raising.
Dataset.bounded_region_readsdocumentsFalsefor a missing entry, and its directory branch only checks that the case directory exists. It then calls this method for the group. If<case>/<group>.h5is absent,get_infosfalls through tofile_to_data, andsitk.ReadTransformraises on the missing path. A routing probe must not raise.🛡️ Proposed fix
def bounded_region_reads(self, name: str) -> bool: + if not os.path.exists(self._path(name)): + return False # priced pessimistically, like the base shape, _attributes = self.get_infos("", name) return len(shape) == 4 and shape[0] == 3🤖 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 2046 - 2048, Update Dataset.bounded_region_reads to detect a missing dataset entry before calling get_infos, returning False when the corresponding case/group path does not exist. Preserve the existing shape validation for present entries and ensure missing paths do not reach file_to_data or raise from sitk.ReadTransform.docs/source/reference/components/transforms.md-100-100 (1)
100-100: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAlign the inverse argument name in the Stream column.
The argument list declares
inverse=True, but the Stream column namesinvert: true. Every other row in this table usesinverse. Use one spelling so a reader can copy the key into YAML.📝 Proposed fix
-a type decomposes into no bounded map, or `invert: true` names a spline or a field | +a type decomposes into no bounded map, or `inverse: true` names a spline or a field |🤖 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 `@docs/source/reference/components/transforms.md` at line 100, Update the Resample row’s Stream column to use the declared argument name “inverse” instead of “invert”, preserving the existing true value and making the YAML key consistent with the argument list.konfai/utils/dataset.py-2050-2078 (1)
2050-2078: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winRemove the float32 cast to maintain dtype consistency with the whole-volume read path.
file_to_data_slicecasts the decoded block tofloat32, butfile_to_datareturnsfloat64throughimage_to_dataof asitkVectorFloat64displacement field. The write path (line 2130) also stores parameters asfloat64. A streamed patch and whole-volume read of the same entry therefore differ in dtype, violating the documented contract.The consumers accept both dtypes without conversion issues (the Mask transform in
transform.pypasses the result totorch.as_tensor, which handles both), so the fix is low effort: change line 2078 fromdtype=np.float32todtype=np.float64to match the stored data and the slow read 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 `@konfai/utils/dataset.py` around lines 2050 - 2078, Update file_to_data_slice so the decoded block matches the float64 dtype returned by file_to_data and stored by the write path; change its final np.asarray conversion from float32 to float64 while preserving the existing slicing and attributes behavior.tests/unit/test_streamed_read_dispatcher.py-283-295 (1)
283-295: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winJustify or relax the bit-exact comparison for the two chained resamples.
Lines 287 and 295 use
torch.equal, which demands bit equality. The chain is two linear resamples, 1.5 mm to 3.0 mm and back to 1.5 mm.The neighbouring composed-region tests at Lines 164 and 175 use
atol=1e-3for the same reason, and the preceding test states its bit-exactness premise explicitly at Lines 265-266: an axis-aligned spacing change reads one axis at a time. This test states no such premise while running two interpolating stages, andkonfai/transformer.py_plan_notesrecords that a streamed non-separable linear resample can differ from a taller-slab run by about 1e-5 of the data range.Add the premise as a comment if bit equality is guaranteed for this chain, or switch to
torch.testing.assert_closewith a stated tolerance.💚 Proposed change
manager.load(manager.transforms, []) reference = _fresh_chain_reference(volume, chain(), manager.dataset._attributes()) - assert torch.equal(manager.data[0], reference) + # Both stages resample one axis at a time on an axis-aligned grid, so the routes agree bit for + # bit; a tolerance here would hide a real divergence. + assert torch.equal(manager.data[0], reference)🤖 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_streamed_read_dispatcher.py` around lines 283 - 295, Relax the bit-exact assertions in this chained resampling test: replace both torch.equal checks for manager.data[0] and streamed with torch.testing.assert_close using an explicit tolerance appropriate for the documented ~1e-5 resampling variation. Keep the existing reference and patch-data comparisons unchanged apart from this numerical comparison behavior.tests/unit/test_api.py-233-239 (1)
233-239: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAdd
config_pathunion coverage totests/unit/test_config.py.The existing dict tests cover
apply_configvalues, not the newPath | str | dictinput accepted byconfigure_workflow_environmentand thebuild_*entry points. Add a test for a dict config tree and its materialized 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 `@tests/unit/test_api.py` around lines 233 - 239, Add a test in the configuration tests covering the Path | str | dict config_path input, using a dict config tree with the workflow root and asserting the resulting materialized path is a file; also verify a missing workflow root raises ConfigError, reusing the existing _materialized_config behavior and test conventions.Source: Path instructions
konfai/utils/utils.py-454-469 (1)
454-469: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winPreserve Windows drive paths with pyramid-level format suffixes
When
split_path_specreceivesC:\Data\D:omezarr@2, it returns("C", "\Data\D", "omezarr@2").DatasetManagervalidates only the base format, then constructsDatasetwith the mangled filename._dataset_leveltherefore fails to match the configured dataset and returns level0instead of2. Preserve the Windows path in the three-part branch or strip the level before format parsing. Add regression coverage.🤖 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/utils.py` around lines 454 - 469, The three-part branch of split_path_spec must preserve Windows drive paths such as C:\Data\D:omezarr@2 instead of splitting the drive prefix into the path components. Detect and reconstruct the Windows path before validating the format and pyramid-level suffix, so DatasetManager receives the original filename and _dataset_level can resolve level 2. Add regression coverage for this Windows path with a level-qualified format.
🧹 Nitpick comments (12)
apps/impact_reg/tests/unit/test_orchestration.py (1)
311-315: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
FileNotFoundErroris the wrong class for "more than one group".This test pins
_find_output_groupto raiseFileNotFoundErrorwhen it finds two groups. Nothing is missing in that case; the layout is ambiguous. A caller that catchesFileNotFoundErrorto mean "the preset produced nothing" now also catches "the preset produced too much".Raise
RuntimeErrorfor the too-many case, asregisterdoes for a group-name disagreement (line 301), and keepFileNotFoundErrorfor the empty case.🤖 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 311 - 315, Update _find_output_group so it raises RuntimeError when multiple output groups are found, matching the existing register behavior for group-name disagreements. Preserve FileNotFoundError exclusively for the no-output-group case, and update test_find_output_group_refuses_more_than_one to expect RuntimeError.apps/impact_reg/impact_reg_konfai/models/convexadam.py (2)
630-630: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse
DIMinstead of the literal3for the channel bound.The file defines
DIMand uses it throughout for the field component count.ChannelSelect(0, 3)hard-codes the same number, so a change ofDIMwould leave this slice wrong.♻️ Derive the bound from `DIM`
- self.add_module("DisplacementField", ChannelSelect(0, 3), in_branch=["registration"], out_branch=["dvf"]) + self.add_module("DisplacementField", ChannelSelect(0, DIM), in_branch=["registration"], out_branch=["dvf"])🤖 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/models/convexadam.py` at line 630, Update the ChannelSelect call in the DisplacementField module registration to use the existing DIM constant as its upper channel bound instead of the hard-coded 3, keeping the registration and branch configuration unchanged.
501-503: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUpdate the surrounding
ConvexAdamRegistrationdocumentation to the single-output contract.Lines 501-502 now stack only the displacement field. Two nearby descriptions still state the old two-output contract:
- The class docstring (line 468) says the module maps to "moved image + DVF on the fixed grid".
- The comment above
fixed_attrs(lines 489-490) says the return value is "the moved image (1 channel) channel-stacked with the displacement field", split by "downstream ChannelSelect modules".Both now describe a tensor this method does not build.
🤖 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/models/convexadam.py` around lines 501 - 503, Update the surrounding documentation for ConvexAdamRegistration to describe the single displacement-field output returned by torch.stack(combined, dim=0). Revise the class docstring and the comment above fixed_attrs to remove references to a moved image, channel-stacking, and downstream ChannelSelect modules while preserving accurate fixed-grid DVF wording.tests/unit/test_resample_to_reference.py (1)
860-874: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the second half of the claim.
Line 867 states that a stage without a field measures nothing, and Line 868 only checks the locality kind.
_stage_regrid_kindnever readsmeasures_at_run, so a regression that returnedTruefor a field-less stage would pass. Add the assertion the comment already promises.🧪 Proposed change
-def _stage_regrid_kind(images: Dataset) -> LocalityKind: +def _stage_regrid_kind(images: Dataset) -> LocalityKind: stage = Resample(reference=_CASE, reference_group="Reference") stage.set_datasets([images]) + assert not stage.measures_at_run return stage.patch_locality(_attributes(_SOURCE_ORIGIN, _SOURCE_SPACING)).kind🤖 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_resample_to_reference.py` around lines 860 - 874, Update _stage_regrid_kind or the field-less portion of test_a_field_with_no_bound_still_streams to also verify that measures_at_run is false for the stage created without a field, while preserving the existing LocalityKind.REGRID assertion.konfai/data/transform.py (1)
1703-1704: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSay why
Sizeis pushed twice.Two consecutive assignments to the same key read as a redundant overwrite. They are not:
Attribute.__setitem__stacks the values, and_inverse_geometrypops both — the top for the target extent it discards, the one beneath for the extent the inverse restores. A reader who deletes Line 1703 breaks the inverse silently, becausepopthen raisesNameErrorandinverse_patch_localitydegrades toWHOLE_VOLUMEinstead of failing.♻️ Proposed comment
+ # Two entries, and the stack is the contract: `_inverse_geometry` pops the target extent it + # is holding and returns the source extent under it. Dropping either push makes the inverse + # unable to state the shape it restores. cache_attribute["Size"] = np.asarray(shape) cache_attribute["Size"] = np.asarray([int(extent) for extent in target.size_zyx])🤖 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 1703 - 1704, Document the intentional double assignment to cache_attribute["Size"] in the transform flow: Attribute.__setitem__ stacks both values, and _inverse_geometry pops them in reverse order, discarding the target extent and restoring the original shape extent. Explain that both assignments must remain to preserve inverse_patch_locality behavior.tests/unit/test_itk_transform_backend.py (1)
88-96: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a case for the refused region shape.
_ItkTransformDataStream.write_sliceraisesDatasetManagerErrorwhen a region is not a full-width leading-axis slab (konfai/utils/dataset.pyLines 774-778). No test reaches that branch, so a future change to the offset arithmetic could silently accept a partial-width region and write the parameters at the wrong offsets.💚 Proposed test
def test_a_partial_width_region_is_refused(tmp_path: Path) -> None: """The offset arithmetic holds only for full-width leading-axis slabs, so anything else refuses.""" dataset = Dataset(tmp_path / "out", "itktransform") stream = dataset.open_data_stream("Transform", "P000", [3, 4, 5, 6], np.dtype("float32"), _attributes()) assert stream is not None try: with pytest.raises(DatasetManagerError, match="full-width"): stream.write_slice((slice(0, 3), slice(0, 2), slice(0, 5), slice(0, 3)), _field()[:, 0:2, :, 0:3]) finally: stream.abort()🤖 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_itk_transform_backend.py` around lines 88 - 96, Add a test alongside test_an_aborted_stream_leaves_no_entry that opens the same Transform stream and attempts a partial-width leading-axis region. Assert write_slice raises DatasetManagerError with a “full-width” message, then always call stream.abort() in cleanup.tests/unit/test_data_stream.py (1)
300-306: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSkip these two tests when SimpleITK is absent.
SimpleITKis an optional heavy dependency in this project. A plainimport SimpleITK as sitkmakes both new tests fail with a collection error in an environment installed without the imaging extra, where the rest of this module still runs. Usepytest.importorskip, astests/unit/test_itk_transform_backend.pydoes at its Line 29.♻️ Proposed change
- import SimpleITK as sitk + sitk = pytest.importorskip("SimpleITK")Apply the same change at Line 333.
As per coding guidelines: "Optional heavy dependencies such as
SimpleITK,h5py,pydicom, andzarrmust be imported lazily and raise an actionable install hint at point of use."🤖 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_data_stream.py` around lines 300 - 306, Replace the direct SimpleITK imports in both new tests, including test_nii_stream_is_the_file_sitk_would_have_written and the test around line 333, with pytest.importorskip so the tests are skipped when the optional dependency is unavailable. Follow the existing pattern in test_itk_transform_backend.py and retain the local sitk alias for the test bodies.Source: Coding guidelines
tests/unit/test_transformer_workflow.py (1)
959-1008: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueName the format-token mismatch in the test.
_write_configdeclares the source as{source}:mha, while this test writes the case as.nii.gz. The test passes only becauseSitkFileauto-detects the entry's real extension and the token carries the write format. That dependency is the point of the test, so state it. A reader who does not know the rule reads the:mhatoken as a bug.♻️ Proposed change
rng = np.random.default_rng(3) + # The config's ':mha' token carries the WRITE format only: the source entry is read through + # the backend's own extension probe, which is what puts a gzipped NIfTI on the read side here. source = Dataset(tmp_path / "source", "nii.gz")🤖 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_transformer_workflow.py` around lines 959 - 1008, Clarify the test setup around _write_config to explicitly state that the configuration token uses :mha while the source is written as .nii.gz, and that SitkFile auto-detects the entry’s actual extension while the token supplies the write format. Preserve the existing mismatch because it is the behavior under test, but document it so readers do not interpret :mha as an error.konfai/data/patching.py (2)
1836-1841: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDeclare the measured-pull contract on the
Stageprotocol.
run_pullis built only when the stage carries bothmeasured_region_sourceand a truthymeasures_at_run. Neither name appears in theStageprotocol at Lines 100-118, so the pairing is discoverable only from thisgetattrpair.A stage that implements
measured_region_sourceand omitsmeasures_at_runsilently keeps the headers-onlypullat run time. That is the wrong window for a stage that sizes its windows from the data it reads, and this file already states the consequence at Line 270: a short window returns the fill instead of raising.Add both members to the protocol as optional, or read them through one named helper that fails loudly when only one is present.
🤖 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/patching.py` around lines 1836 - 1841, Update the Stage protocol to explicitly declare optional measured_region_source and measures_at_run members, ensuring stages that provide the measured source also declare the run-time measurement flag. Preserve the existing run_pull construction in the surrounding patching logic.
2310-2329: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winPrice one representative slab instead of every slab.
The docstring at Lines 2279-2280 says the estimate is "one representative slab priced through the plan's own pull maps". The loop prices every slab: it iterates
range(0, landed[0], rows)and evaluates each stage's pull per iteration.When the budget drives
rowsto its floor of 1, this runslanded[0]iterations times the stage count, and each_RemapPull.__call__allocates a freshlistandAttribute. That cost lands on the planner, which is meant to stay headers-only and cheap, and it is paid once per case per plan.Either price one interior slab and scale it by the slab count, or update the docstring to state that every slab is priced.
🤖 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/patching.py` around lines 2310 - 2329, Update _segment_read_factor to price a single representative slab through the reversed plans, then scale that read cost by the number of slabs instead of iterating over every slab. Preserve the bounded and unbounded read behavior, and choose an interior slab shape consistent with the existing rows and landed dimensions.konfai/utils/runtime.py (1)
205-208: 🗄️ Data Integrity & Integration | 🔵 Trivial | 💤 Low valueConsider the atomic-write rule for this config write.
The coding guidelines require configuration files to be written through a temporary file followed by
os.replace. This write targetspathdirectly.The exposure looks closed here:
scratchis a freshmkdtempdirectory, and the caller exports the path only after this function returns, so no reader can observe the partial file. Confirm that reasoning is deliberate, or route the write throughos.replaceso the rule holds without a case-by-case argument.As per coding guidelines: "Write configuration files atomically using a temporary file followed by
os.replace, so readers never observe truncated or all-default configuration."🤖 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/runtime.py` around lines 205 - 208, Update the configuration write in the function producing path from scratch so it follows the atomic-write rule: dump the YAML to a temporary file in the same directory, then publish it with os.replace to the final path. Preserve returning the final path and ensure the temporary file is cleaned up or safely handled on failure.konfai/__init__.py (1)
394-410: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd a lazy-export contract test.
All seven registry names exist in
konfai.api, but no test asserts this mapping. Add a focused test for the registry and module exports.🤖 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/__init__.py` around lines 394 - 410, Add a focused test covering _API_EXPORTS and module-level lazy resolution through __getattr__: verify every registered name exists in konfai.api and resolves to the same object when accessed from konfai, while preserving AttributeError for unregistered names.
🤖 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 554-562: Update _derive_moved to reject or otherwise refuse
processing when the resolved output group parameter group is named "Moved",
before the loop calls _output_path. Preserve the existing behavior for other
group names and ensure no input transform is deleted or staged after the
collision is detected.
- Line 447: Update _infer_preset and its caller to preserve the original
input-to-case ordering for cohorts containing P1000. Return an ordered case
mapping from _infer_preset, then use that mapping at the cases assignment
instead of sorted(fields_by_preset[presets[0]]), so the moving-unit pairing at
line 490 remains aligned with the input order.
In `@konfai/api.py`:
- Around line 379-404: Preserve path-based configuration files in both predict
and train by snapshotting their bytes before build_predict/build_train and
restoring the original contents after workflow construction, including when
construction raises. Keep dict-based configurations unchanged, and preserve the
documented guarantee that callers’ source Config.yml or Prediction.yml files
remain untouched.
- Around line 91-98: Update _yaml_safe to handle np.generic values before the
built-in bool, int, float, and str checks, returning value.item() for NumPy
scalars; preserve the existing handling for Python scalars and Path values.
In `@konfai/data/reduction.py`:
- Around line 165-167: Set the Std reduction class’s working_multiple to the
number of additional buffers allocated by _mean, _m2, delta, value - self._mean,
and the optional tensor.float() copy, replacing the inherited 0.0 value. Keep
voxel_local and incremental unchanged.
In `@konfai/utils/dataset.py`:
- Around line 2091-2117: Update data_to_file and open_data_stream to derive
their write/final target directly from the canonical HDF5 filename instead of
self._path(name), which may resolve an existing .tfm read path. Preserve _path
for read probing, but ensure all HDF5 transform output is written and renamed to
the .h5 target.
---
Outside diff comments:
In `@konfai/data/patching.py`:
- Around line 1477-1499: Update the Expand handling around stage.state_init and
_adopt_case_facts so all attribute updates produced by state_init, including
Spacing, Origin, and Direction, are preserved in each copy’s attributes rather
than only box. Reuse the existing attribute propagation approach used by
_draw_augmentation_lists, or extend _adopt_case_facts to copy the complete
required state-init attributes.
---
Minor comments:
In `@apps/impact_reg/impact_reg_konfai/impact_reg.py`:
- Around line 614-616: Update the case-count and transform selection logic
around n_cases so transforms must contain either zero entries or exactly one
entry per case; reject any other nonzero length before evaluation, consistent
with register’s mask-count validation. Preserve the existing identity behavior
only when transforms is empty, and ensure every evaluated case uses its
positional transform when transforms are provided.
- Around line 743-751: Move the minimum-member validation in the uncertainty
setup around `_units` so it checks the expanded `members` collection rather than
raw `dvfs`. Expand `dvfs` first, then raise the existing ValueError when
`len(members) < 2`, before using `members[0]` or building the stage group.
In `@apps/impact_reg/README.md`:
- Line 87: Update the register documentation to name the accepted --uncertainty
flag instead of --keep_dvf, and add both --uncertainty and --fields_only to the
register arguments table. Keep the descriptions aligned with how _dispatch
forwards these parsed options as keep_dvf and fields_only.
In `@apps/impact_reg/tests/unit/test_displacement_field_io.py`:
- Around line 223-235: Gate
test_ensemble_field_written_by_the_orchestrator_is_a_declared_field on
_zarr_v3_available() before creating stores or invoking _ensemble_mean. Use the
same skip mechanism and reason as the comparable gate in test_orchestration.py,
preserving the test’s existing assertions and behavior when zarr v3 is
available.
In `@docs/source/config_guide/transform.md`:
- Around line 279-303: Remove the stale refusal-list bullet that says a
reference with a differing Direction is refused or requires Canonical first.
Update the refusal list for the unified Resample documentation so it matches the
implemented rotation-aware behavior and the later list that already omits this
restriction.
In `@docs/source/reference/components/storage-backends.md`:
- Around line 27-30: Update the imaging installation tip to list all six
dependencies, adding dask and specifying that ngff-zarr requires version 0.38 or
later while preserving the existing SimpleITK, h5py, pydicom, and zarr entries.
In `@docs/source/reference/components/transforms.md`:
- Line 100: Update the Resample row’s Stream column to use the declared argument
name “inverse” instead of “invert”, preserving the existing true value and
making the YAML key consistent with the argument list.
In `@docs/source/usage/large-images.md`:
- Around line 195-205: Update the documentation paragraph around the streamed
resampling guarantees to match existing test coverage, or add tests covering
each stated guarantee: bit-identical axis-aligned streamed versus whole-volume
output and near-tied blended-logit argmax behavior. Ensure tolerance claims
consistently reflect the field, stored-transform, and integration tests,
especially the documented 1e-5 range-relative value.
In `@docs/source/usage/python-workflows.md`:
- Around line 44-45: Update the `plan_transform` documentation to say it accepts
the same planning arguments as `transform`, while explicitly noting that
execution-only arguments such as `gpu` and `quiet` are not supported. Preserve
the explanation that it returns a `TransformPlan` without executing anything.
In `@konfai/api.py`:
- Around line 227-230: The dataset_options merge can overwrite dedicated dataset
arguments in both workflow builders. In konfai/api.py lines 227-230 within
_transform_tree, reject or raise before merging any dataset_options key
colliding with dataset_filenames, groups_src, or memory_budget; in konfai/api.py
lines 357-358 within evaluate, apply the same collision refusal for
dataset_filenames and groups_src. Preserve the existing merges for
non-conflicting options.
In `@konfai/metric/measure.py`:
- Around line 78-83: Add configuration-binding coverage in
tests/unit/test_config.py for Criterion constructors, using the relevant
Criterion subclasses and their __init_subclass__ behavior. Verify serialization
preserves explicit positional arguments, keyword arguments, and default values,
and include a subclass whose constructor delegates to super().__init__.
In `@konfai/utils/dataset.py`:
- Around line 2136-2141: Update H5 file entry listing in get_names and get_group
to exclude staging leftovers whose filenames match the hidden
.<name>.<pid>.tmp.h5 pattern, while retaining valid .h5 and .tfm entries and
existing sorting behavior.
- Around line 2046-2048: Update Dataset.bounded_region_reads to detect a missing
dataset entry before calling get_infos, returning False when the corresponding
case/group path does not exist. Preserve the existing shape validation for
present entries and ensure missing paths do not reach file_to_data or raise from
sitk.ReadTransform.
- Around line 2050-2078: Update file_to_data_slice so the decoded block matches
the float64 dtype returned by file_to_data and stored by the write path; change
its final np.asarray conversion from float32 to float64 while preserving the
existing slicing and attributes behavior.
In `@konfai/utils/utils.py`:
- Around line 454-469: The three-part branch of split_path_spec must preserve
Windows drive paths such as C:\Data\D:omezarr@2 instead of splitting the drive
prefix into the path components. Detect and reconstruct the Windows path before
validating the format and pyramid-level suffix, so DatasetManager receives the
original filename and _dataset_level can resolve level 2. Add regression
coverage for this Windows path with a level-qualified format.
In `@tests/unit/test_api.py`:
- Around line 233-239: Add a test in the configuration tests covering the Path |
str | dict config_path input, using a dict config tree with the workflow root
and asserting the resulting materialized path is a file; also verify a missing
workflow root raises ConfigError, reusing the existing _materialized_config
behavior and test conventions.
In `@tests/unit/test_resample.py`:
- Line 33: Remove the module-level SimpleITK import gate from
tests/unit/test_resample.py so test_an_axis_the_map_leaves_alone_is_left_alone
runs with only its NumPy and torch dependencies; if future tests require
SimpleITK, apply pytest.importorskip locally within those tests or their fixture
instead.
In `@tests/unit/test_streamed_read_dispatcher.py`:
- Around line 283-295: Relax the bit-exact assertions in this chained resampling
test: replace both torch.equal checks for manager.data[0] and streamed with
torch.testing.assert_close using an explicit tolerance appropriate for the
documented ~1e-5 resampling variation. Keep the existing reference and
patch-data comparisons unchanged apart from this numerical comparison behavior.
---
Nitpick comments:
In `@apps/impact_reg/impact_reg_konfai/models/convexadam.py`:
- Line 630: Update the ChannelSelect call in the DisplacementField module
registration to use the existing DIM constant as its upper channel bound instead
of the hard-coded 3, keeping the registration and branch configuration
unchanged.
- Around line 501-503: Update the surrounding documentation for
ConvexAdamRegistration to describe the single displacement-field output returned
by torch.stack(combined, dim=0). Revise the class docstring and the comment
above fixed_attrs to remove references to a moved image, channel-stacking, and
downstream ChannelSelect modules while preserving accurate fixed-grid DVF
wording.
In `@apps/impact_reg/tests/unit/test_orchestration.py`:
- Around line 311-315: Update _find_output_group so it raises RuntimeError when
multiple output groups are found, matching the existing register behavior for
group-name disagreements. Preserve FileNotFoundError exclusively for the
no-output-group case, and update test_find_output_group_refuses_more_than_one to
expect RuntimeError.
In `@konfai/__init__.py`:
- Around line 394-410: Add a focused test covering _API_EXPORTS and module-level
lazy resolution through __getattr__: verify every registered name exists in
konfai.api and resolves to the same object when accessed from konfai, while
preserving AttributeError for unregistered names.
In `@konfai/data/patching.py`:
- Around line 1836-1841: Update the Stage protocol to explicitly declare
optional measured_region_source and measures_at_run members, ensuring stages
that provide the measured source also declare the run-time measurement flag.
Preserve the existing run_pull construction in the surrounding patching logic.
- Around line 2310-2329: Update _segment_read_factor to price a single
representative slab through the reversed plans, then scale that read cost by the
number of slabs instead of iterating over every slab. Preserve the bounded and
unbounded read behavior, and choose an interior slab shape consistent with the
existing rows and landed dimensions.
In `@konfai/data/transform.py`:
- Around line 1703-1704: Document the intentional double assignment to
cache_attribute["Size"] in the transform flow: Attribute.__setitem__ stacks both
values, and _inverse_geometry pops them in reverse order, discarding the target
extent and restoring the original shape extent. Explain that both assignments
must remain to preserve inverse_patch_locality behavior.
In `@konfai/utils/runtime.py`:
- Around line 205-208: Update the configuration write in the function producing
path from scratch so it follows the atomic-write rule: dump the YAML to a
temporary file in the same directory, then publish it with os.replace to the
final path. Preserve returning the final path and ensure the temporary file is
cleaned up or safely handled on failure.
In `@tests/unit/test_data_stream.py`:
- Around line 300-306: Replace the direct SimpleITK imports in both new tests,
including test_nii_stream_is_the_file_sitk_would_have_written and the test
around line 333, with pytest.importorskip so the tests are skipped when the
optional dependency is unavailable. Follow the existing pattern in
test_itk_transform_backend.py and retain the local sitk alias for the test
bodies.
In `@tests/unit/test_itk_transform_backend.py`:
- Around line 88-96: Add a test alongside test_an_aborted_stream_leaves_no_entry
that opens the same Transform stream and attempts a partial-width leading-axis
region. Assert write_slice raises DatasetManagerError with a “full-width”
message, then always call stream.abort() in cleanup.
In `@tests/unit/test_resample_to_reference.py`:
- Around line 860-874: Update _stage_regrid_kind or the field-less portion of
test_a_field_with_no_bound_still_streams to also verify that measures_at_run is
false for the stage created without a field, while preserving the existing
LocalityKind.REGRID assertion.
In `@tests/unit/test_transformer_workflow.py`:
- Around line 959-1008: Clarify the test setup around _write_config to
explicitly state that the configuration token uses :mha while the source is
written as .nii.gz, and that SitkFile auto-detects the entry’s actual extension
while the token supplies the write format. Preserve the existing mismatch
because it is the behavior under test, but document it so readers do not
interpret :mha as an error.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
np.float64 passed the float check unspelled and ruamel refused it at dump time; the numpy branch now comes first. And a config given as a PATH was rewritten in place by resolution write-back -- the caller's file is not this call's to rewrite, so the write-back lands on a scratch copy, removed at exit. Both pinned.
data_to_file resolved an existing .tfm and renamed HDF5 content onto it; ITK selects transform IO from the extension, so that entry was corrupt. The write now always lands on the .h5 name and drops the .tfm it replaces. Std declared working_multiple 0 while holding five buffers beside the region it reads.
sorted() places P1000 before P101 and pairs the wrong moving unit; cases now sort by konfai-apps' own numbering (length, then name). A preset whose output group is named Moved would be deleted by the derivation's own stale-output purge, and is refused by name.
1 participant
The whole transform-release line, as one linear PR. The stack (#91, #93–#97, #99) was reviewed slice by slice — CI green, every CodeRabbit thread addressed — then a top-down merge cascaded the slices into each other instead of into main and GitHub closed the intermediates. This PR now carries the identical, byte-verified 36-commit line against main; the leftover
feat/impact-reg-cases-and-fieldsandfeat/itktransform-backendbranches hold the same tree plus cascade merge commits and can be deleted after this merges.One
Resample(stored transforms decoded and streamed, windows measured at run, no bound to declare), the workflows as Python callables, the:itktransformDataset backend + streamed NIfTI writes, the impact-reg orchestrator fully out-of-core with presets that declare only their transform, and the review cycle's fixes across API, sampling, planning and docs.Merge with "Rebase and merge" (or a merge commit if you prefer a single landmark) — the history is already linear.