feat(py): add Trim, a selection combine that keeps the most central patch - #70
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughPatch reassembly now supports position-aware per-axis weighting, boundary-aware discrete ChangesPatch reassembly behavior
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant OutputDataset
participant Trim
participant Accumulator
participant ResultTensor
OutputDataset->>Trim: configure default patch combiner
Accumulator->>Trim: request position-aware window
Trim-->>Accumulator: return weighted or kept-region window
Accumulator->>ResultTensor: write kept region or weighted patch
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.
🧹 Nitpick comments (1)
tests/unit/test_patching.py (1)
484-497: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winExercise actual arrival order for last-write-wins.
The test currently inserts patches in spatial/index order, so it does not distinguish arrival-order semantics from a spatially ordered implementation. Add a reversed or non-spatial insertion order and assert that the final overlap reflects the last-added patch.
🤖 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_patching.py` around lines 484 - 497, Update test_unweighted_overlap_is_last_write_wins to add patches in a reversed or otherwise non-spatial order instead of enumerating slices sequentially, then adjust the expected output so each overlap reflects the last-added patch’s value. Keep the test focused on validating arrival-order last-write-wins behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@tests/unit/test_patching.py`:
- Around line 484-497: Update test_unweighted_overlap_is_last_write_wins to add
patches in a reversed or otherwise non-spatial order instead of enumerating
slices sequentially, then adjust the expected output so each overlap reflects
the last-added patch’s value. Keep the test focused on validating arrival-order
last-write-wins behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f22eb935-a19d-4196-8359-145308b06881
📒 Files selected for processing (1)
tests/unit/test_patching.py
vboussot
force-pushed
the
feat/combine-selection-contract
branch
from
July 29, 2026 00:12
08a0588 to
6132401
Compare
vboussot
changed the title
There was a problem hiding this comment.
🧹 Nitpick comments (2)
konfai/data/patching.py (1)
525-550: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMinor: ternary-as-statement in the per-axis share multiply is unconventional.
torch.mul(data, share, out=self._weighted) if dim == 0 else self._weighted.mul_(share)Using a conditional expression purely for its side effect works but reads awkwardly. An explicit
if/elsewould be clearer without changing behavior.♻️ Optional readability tweak
for dim, s in enumerate(patch_slice): view = [1] * data.ndim view[self._n + dim] = -1 share = self._share(dim, s.start, data).view(view) - torch.mul(data, share, out=self._weighted) if dim == 0 else self._weighted.mul_(share) + if dim == 0: + torch.mul(data, share, out=self._weighted) + else: + self._weighted.mul_(share)🤖 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 525 - 550, Replace the side-effect-only conditional expression in _weighted_patch with an explicit if/else: initialize self._weighted using torch.mul(data, share, out=self._weighted) for dim == 0, and apply self._weighted.mul_(share) for subsequent dimensions, preserving the existing behavior.tests/unit/test_patching.py (1)
564-579: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winConsider also covering a non-exact-tiling (truncated last patch) geometry for
Trim.This test and its sibling only use shapes/patch sizes/overlaps that tile exactly (every patch reaches full
patch_size).Trim's binary 0/1 selection is the most fragile combine to a coverage gap (a weighted combine likeCosinus/Gaussianalways has a strictly-positive total, so it can't silently produce a0/0division;Trimcould, if the abutting-band invariant were ever violated by a future change). A regression test with a shape that doesn't divide evenly (e.g.shape=[9],patch=[4],overlap=2) would pin the truncated-last-patch case that this suite currently doesn't exercise forTrim.🤖 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_patching.py` around lines 564 - 579, Extend test_trim_reassembles_exactly_and_keeps_values_discrete to cover a non-exact tiling case, such as shape [9], patch [4], and overlap 2, where the final patch is truncated. Verify Trim through Accumulator.assemble still reconstructs the original labels exactly and preserves discrete values, while retaining the existing exact-tiling coverage.
🤖 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.
Nitpick comments:
In `@konfai/data/patching.py`:
- Around line 525-550: Replace the side-effect-only conditional expression in
_weighted_patch with an explicit if/else: initialize self._weighted using
torch.mul(data, share, out=self._weighted) for dim == 0, and apply
self._weighted.mul_(share) for subsequent dimensions, preserving the existing
behavior.
In `@tests/unit/test_patching.py`:
- Around line 564-579: Extend
test_trim_reassembles_exactly_and_keeps_values_discrete to cover a non-exact
tiling case, such as shape [9], patch [4], and overlap 2, where the final patch
is truncated. Verify Trim through Accumulator.assemble still reconstructs the
original labels exactly and preserves discrete values, while retaining the
existing exact-tiling coverage.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ec08bae7-23e4-4fe0-8b75-ea53cd58ea5c
📒 Files selected for processing (2)
konfai/data/patching.pytests/unit/test_patching.py
…atch Without a combine an overlapped voxel keeps whichever patch wrote last. That is a choice, not a consequence, and an arbitrary one: the winning patch may hold that voxel on its own border, with no context behind it, which is what shows up as a seam. Trim keeps each patch's central `patch - overlap` band instead, so the kept bands tile every axis exactly and the weights already sum to one -- there is nothing to normalise. The first and last patch of an axis open to the volume edge, so the tiling has no gap there either. Every kept voxel then carries at least half the overlap of context on each side. The values are 0 or 1, so the patch is selected rather than averaged: a label map or an argmax survives reassembly, where any weighting invents values between its classes. The window is now asked for per grid position, because a selection has to open its border patches while a weighting returns the same window everywhere. Per-axis overlaps are kept alongside their max for the same reason. Also covers three properties of the reassembly the suite did not pin: that an anisotropic overlap reassembles exactly, that a weighted blend does not depend on the order patches arrive in, and the last-write-wins semantics itself -- so any future change to it is visible in the diff.
A patch grid overlaps whether or not a combine is declared, and an undeclared one left the overlap to whichever patch wrote last -- an arbitrary winner, possibly holding that voxel on its own border with no context behind it, which is what a seam is. The default is now Trim: each patch keeps its central band, the bands tile the volume exactly, and nothing is averaged, so a label map still survives reassembly. Assembly takes the shorter path for it. A selection declares itself through PathCombine.selects, and _blend then writes the box a patch owns instead of scaling by a 0/1 mask and accumulating -- the same values for less work. Measured on a 128x384x384 volume, patch [64,192,192], 3 channels, CUDA: last-write-wins (plain assign) 2.4 ms Trim 3.3 ms Cosinus (a reduction) 9.8 ms The kept box is read once from the host-side windows and cached per grid position: deriving it from the device share instead syncs the host on every patch, which cost 25.4 ms -- more than the weighting it replaces. This changes the output of any configuration that declared no combine. An explicit Mean/Cosinus/Gaussian is untouched.
An overlap at least as wide as the patch leaves nothing between the two trimmed sides, so the window came out all zeros and the box derivation raised IndexError on an empty nonzero(). The patch is kept whole instead: that axis stops being a partition and falls back to last-write-wins on it, which is what it was before. Reachable from any direct caller of set_patch_config -- the slicing plan refuses overlap >= patch_size, but nothing guards the blend window built beside it. Proves the property the selection path rests on, over seven geometries (1-D, no axis divisible by the stride, a different overlap per axis including none, a patch larger than the volume, no overlap, a single patch, an odd overlap): the kept boxes cover every voxel exactly once. An off-by-one in the kept band fails five of the seven, so the test discriminates. Free axes now run Trim too.
…y out Review follow-up. The reassembly and partition tests only used geometries that tile exactly, and a 0/1 selection is what a coverage gap hurts most: a weighting always has a strictly positive total, so a gap only darkens a voxel, where a selection leaves it unwritten and nothing reports it. Both now also run a truncated last patch. The geometry matters more than it looks: with shape 9, patch 4, overlap 2 the volume clamp already cuts where the last patch's tail would have been opened, so that case never exercises the tail and passes with the opening removed. Shape 9/11, patch 6, overlap 4 does exercise it -- disabling the opening fails both tests, so they discriminate. Also replaces the side-effect-only conditional expression in the per-axis share multiply with an explicit if/else. Same behaviour.
Defaulting to Trim made this reachable: the caller drops the axes of extent 1, so a case with nothing tiled arrives as an EMPTY patch size, not a missing one, and `[] is not None` let it through. A blend window built on no axis then left max() nothing to take and the prediction died on `max() iterable argument is empty`. Before the default, patch_combine was None on those cases and set_patch_config was never reached -- the guard had simply never been asked the question. An empty patch size is a single patch covering the volume, so no combine applies: it now takes the same branch as a missing one. Caught by test_auto_patch_restart_is_voxel_identical_to_whole_volume, which the unit suite does not cover.
vboussot
force-pushed
the
feat/combine-selection-contract
branch
from
July 29, 2026 15:21
3ff278e to
e928a75
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Stacked on #69 — review that one first.
The problem
Without a combine, an overlapped voxel keeps whichever patch wrote last. That is a choice, not a
consequence, and an arbitrary one: the winning patch may hold that voxel on its own border, with no
context behind it. That is what a seam is.
Trim
Each patch keeps its central
patch - overlapband. Those bands tile every axis exactly, so theweights already sum to one and there is nothing to normalise; the first and last patch of an axis open
to the volume edge, so the tiling has no gap there either. Every kept voxel then carries at least half
the overlap of context on each side.
Volume 8, patch 4, overlap 2 (starts 0, 2, 4), each patch carrying its own index:
The values are 0 or 1, so the patch is selected, not averaged — a label map or an argmax survives
reassembly, where any weighting invents values between its classes. Verified on
int64,uint8andfloat32.It is now the default
patch_combineleft undeclared resolves toTriminstead of last-write-wins. This changes theoutput of any configuration that declared no combine; an explicit
Mean/Cosinus/Gaussianisuntouched. Separate commit, so it can be dropped on its own.
Assembly takes the shorter path for it: a selection declares itself through
PathCombine.selects, and_blendwrites the box a patch owns instead of scaling by a 0/1 mask and accumulating. On a128×384×384 volume, patch [64,192,192], 3 channels, CUDA:
The kept box is read once from the host-side windows and cached per grid position. Deriving it from
the device share instead syncs the host on every patch — that first version measured 25.4 ms, more
than the weighting it replaced.
What it needed
The window is now asked for per grid position: a selection opens its border patches, a weighting
returns the same window everywhere. Per-axis overlaps are kept alongside their max for the same
reason. Both additive; every existing combine takes the default.
Tested
The partition is the property everything rests on — a gap leaves voxels unwritten and nothing
reports it. Seven geometries (1-D, no axis divisible by the stride, a different overlap per axis
including none, a patch larger than the volume, no overlap, a single patch, an odd overlap, a
truncated last patch) assert every voxel is written exactly once. An off-by-one in the kept band fails
five of them; disabling the last patch's tail opening fails six.
The truncated geometry is chosen deliberately: with shape 9, patch 4, overlap 2 the volume clamp
already cuts where the tail would have been opened, so that case passes even with the opening removed.
Shape 9/11, patch 6, overlap 4 exercises it.
Also fixes a crash found on the way: an overlap at least as wide as the patch left no central band, so
the window came out all zeros and the box derivation raised
IndexErroron an emptynonzero(). Thepatch is kept whole there instead.
Plus three properties of the reassembly the suite did not pin: an anisotropic overlap reassembles
exactly, a weighted blend does not depend on the order patches arrive in, and the last-write-wins
semantics itself.
Not done
No quality measurement of the new default on a real model — what is established is that the tiling is
exact and that discrete values survive, not that the result is better.
Summary by CodeRabbit
New Features
Trim) that keeps a central interior region when reconstructing discrete/label-like outputs.Trimautomatically when no patch-combine strategy is specified.Bug Fixes
patch_sizeas “no patch tiling” (skips patch combining).Tests