feat(transform): resample onto a reference grid, and warp in one pass - #85
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThe PR adds ChangesRegridding and planning
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Sequence Diagram(s)sequenceDiagram
participant TransformPlan
participant ResampleToReference
participant ReferenceDataset
participant SourceDataset
TransformPlan->>ResampleToReference: plan target grid and locality
ResampleToReference->>ReferenceDataset: read reference geometry
ResampleToReference->>SourceDataset: read source regions and displacement field
ResampleToReference-->>TransformPlan: return streamed resampled regions
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
ee4a0e6 to
4de5b91
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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/config_guide/transform.md`:
- Around line 460-475: Remove the duplicate “Statistics a stage may ask for”
subsection and fold only its fallback rule into the existing “Statistics a stage
can ask for” section near the statistics table. Preserve the table’s complete
per-component statistics list, and state that when a stage receives the whole
volume through a fallback path, it should compute the statistic from that tensor
and record it so both execution paths produce the same state.
In `@konfai/data/transform.py`:
- Around line 2380-2385: Update the bound-discovery loop around group_for and
root._root_for so it scans every root that contains the group instead of using
only the root returned for None. For each matching root, enumerate its entries
and collect the maximum bound across all fields, while preserving the existing
handling for unreadable fields and roots without the group.
In `@konfai/transformer.py`:
- Around line 550-552: Update the stage-planning flow around plan_note() to pass
each stage its effective input geometry rather than manager.base_shape and
manager.stored_attributes. Walk the chain using copied shape and Attribute
state, applying each preceding stage’s transform_shape() before planning the
next stage, and preserve the draw-specific geometry when an Expand stage is
present.
In `@tests/unit/test_per_component_statistic.py`:
- Around line 40-61: Implement an identity transform_shape() method on
_CentreAndScale that returns the input shape unchanged, preserving all spatial
geometry metadata while retaining the existing __call__ behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: fc094c4b-c232-40c1-9868-be4c08e97cd0
📒 Files selected for processing (9)
docs/source/config_guide/transform.mdkonfai/data/data_manager.pykonfai/data/patching.pykonfai/data/transform.pykonfai/transformer.pytests/integration/test_transform_doc_examples.pytests/unit/test_per_component_statistic.pytests/unit/test_resample_to_reference.pytests/unit/test_transform_locality_contract.py
1632550 to
e34000f
Compare
e511de0 to
3016ac3
Compare
e34000f to
d02fe1e
Compare
3016ac3 to
32a93c7
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/integration/test_transform_doc_examples.py (1)
147-147: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse the repository text-I/O policy.
Remove the isolated
encoding="utf-8"arguments. The generated configuration is read later with the platform-default encoding. Keep the test and workflow on the same policy.Based on learnings, use the repository's platform-default text encoding consistently for text I/O and do not add isolated
encoding="utf-8"usage.Also applies to: 158-160, 169-169
🤖 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/integration/test_transform_doc_examples.py` at line 147, Update the text I/O calls in the test around DOC.read_text and the additionally referenced lines to remove isolated encoding="utf-8" arguments, using the repository’s platform-default text encoding consistently for both the test input and generated configuration reads.Source: Learnings
🤖 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 `@tests/unit/test_transformer_workflow.py`:
- Around line 372-374: Update the test description docstring in the resampling
workflow test to state that the complementary output fill fraction is 32.5%,
matching the 67.5% reference coverage expected near line 386; remove the
inaccurate “two thirds” wording without changing test behavior.
---
Nitpick comments:
In `@tests/integration/test_transform_doc_examples.py`:
- Line 147: Update the text I/O calls in the test around DOC.read_text and the
additionally referenced lines to remove isolated encoding="utf-8" arguments,
using the repository’s platform-default text encoding consistently for both the
test input and generated configuration reads.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2c6fbd6b-f82f-42ea-8368-24fdf3664af9
📒 Files selected for processing (10)
docs/source/config_guide/transform.mdkonfai/data/data_manager.pykonfai/data/patching.pykonfai/data/transform.pykonfai/transformer.pytests/integration/test_transform_doc_examples.pytests/unit/test_per_component_statistic.pytests/unit/test_resample_to_reference.pytests/unit/test_transform_locality_contract.pytests/unit/test_transformer_workflow.py
🚧 Files skipped from review as they are similar to previous changes (5)
- konfai/data/data_manager.py
- tests/unit/test_per_component_statistic.py
- tests/unit/test_transform_locality_contract.py
- tests/unit/test_resample_to_reference.py
- konfai/data/transform.py
A cohort has to be on one grid before it can be folded, and until now that meant a separate pass before the transform. ResampleToReference puts a case on the grid of a declared reference case -- extents, spacing, origin and direction -- so the members of a Reduce meet on a grid that is one of their own rather than an invented one.
The stage takes an optional field, so two passes over a volume become one and the intermediate -- a case resampled onto the reference but not yet warped -- never has to exist on disk. ShapeUpdate goes: it was never generic, and the shared stage covers what it did.
d02fe1e to
a45a091
Compare
32a93c7 to
b2dfd1a
Compare
The auto bound stopped at the first root that answers, but the bound is the cohort's and a field declared by group alone is looked up beside the cases -- which a run may spread over several stores. The plan asked each stage about the case as stored: past a resample a case that covered the whole reference covers 67.5% of it, and the plan said nothing about the third that is fill. And a statistics section contradicted the table above it.
b2dfd1a to
001ecac
Compare
9fcacc8
into
fix/transform-audit-follow-ups
Stacked on #82.
A cohort has to be on one grid before it can be folded, and until now that meant a separate
resampling pass before the transform. Two stages close it:
ResampleToReferenceputs a case on the grid of a declared reference case — extents, spacing,origin and direction — so the members of a
Reducemeet on a grid that is one of their ownrather than an invented one.
field:, which composes the grid change and the displacementinto a single resampling. Two passes over a volume become one, and the intermediate — a case
resampled onto the reference but not yet warped — never has to exist on disk.
The plan grew a notes column for what the stages know about themselves: a nested inference whose
memory nothing can bound, a case that meets only part of the grid it is being resampled onto.
A verdict and a byte count are what the plan computes about a chain; this is what a chain says
about itself.
Rebased onto the tip of the stack. The
Warpbound reading moved into a_DisplacementSourceshared with the new stage, and the guard fix from #80 was carried into the moved copy — its test
(an unreadable entry in a directory-backed field group) passes against the relocated code.
1342 unit tests pass on this branch.
Summary by CodeRabbit
New Features
ResampleToReferencefor resampling data onto a stored reference grid.Documentation
Bug Fixes
ShapeUpdatetransform.