Fix nested Compose map_items in forward and inverse paths#8787
Fix nested Compose map_items in forward and inverse paths#8787aymuos15 wants to merge 9 commits intoProject-MONAI:devfrom
Conversation
…t-MONAI#7932, Project-MONAI#7565) When a child Compose has a different map_items setting than its parent, the parent now delegates to the child instead of expanding list/tuple data itself. This applies to forward execution (apply_transform), flatten(), and the inverse path in Compose, RandomOrder, and SomeOf. Signed-off-by: Soumya Snigdha Kundu <soumya_snigdha.kundu@kcl.ac.uk>
|
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:
📝 WalkthroughWalkthroughAdded a helper Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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)
monai/transforms/compose.py (1)
40-51: Add Google-style docstring with Args/Returns.Per coding guidelines, docstrings should describe parameters and return values.
📝 Proposed docstring
def _inverse_one( t: InvertibleTransform, data: Any, map_items: bool | int, unpack_items: bool, log_stats: bool | str, ) -> Any: - """Invert a single transform, delegating directly to nested ``Compose`` objects.""" + """Invert a single transform, delegating directly to nested ``Compose`` objects. + + Args: + t: The invertible transform to invert. + data: Data to be inverted. + map_items: Whether to map over list/tuple items. + unpack_items: Whether to unpack data as parameters. + log_stats: Logger name or boolean for logging. + + Returns: + The inverted data. + """ if isinstance(t, Compose): return t.inverse(data) return apply_transform(t.inverse, data, map_items, unpack_items, lazy=False, log_stats=log_stats)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@monai/transforms/compose.py` around lines 40 - 51, Add a Google-style docstring to the _inverse_one function describing its purpose, parameters and return value: state that it inverts a single transform (delegating to Compose.inverse when t is a Compose), document args t (InvertibleTransform), data (Any), map_items (bool|int), unpack_items (bool), log_stats (bool|str), and explain the return value (inverted data of type Any); also mention that apply_transform is called for non-Compose transforms with lazy=False. Include short notes on side effects (calls to Compose.inverse/apply_transform) if relevant.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@monai/transforms/compose.py`:
- Around line 40-51: Add a Google-style docstring to the _inverse_one function
describing its purpose, parameters and return value: state that it inverts a
single transform (delegating to Compose.inverse when t is a Compose), document
args t (InvertibleTransform), data (Any), map_items (bool|int), unpack_items
(bool), log_stats (bool|str), and explain the return value (inverted data of
type Any); also mention that apply_transform is called for non-Compose
transforms with lazy=False. Include short notes on side effects (calls to
Compose.inverse/apply_transform) if relevant.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 13d05d32-401c-4be3-a001-687e58886c54
📒 Files selected for processing (3)
monai/transforms/compose.pymonai/transforms/transform.pytests/transforms/compose/test_compose.py
Address CodeRabbit review comment: document Args/Returns for the _inverse_one helper function per project coding guidelines. Signed-off-by: Soumya Snigdha Kundu <soumya_snigdha.kundu@kcl.ac.uk>
Signed-off-by: Soumya Snigdha Kundu <soumya_snigdha.kundu@kcl.ac.uk>
Signed-off-by: Soumya Snigdha Kundu <soumya_snigdha.kundu@kcl.ac.uk>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
monai/transforms/compose.py (1)
354-361:flatten()doesn't preserve parent attributes (unlikeOneOf.flatten()).Line 361 returns
Compose(new_transforms)with defaults. If someone callsflatten()expecting an equivalent executable pipeline,map_items,unpack_items,log_stats,lazy, andoverridesare lost.Current internal usage only reads
.transforms, so no bug today.Proposed fix
- return Compose(new_transforms) + return Compose(new_transforms, self.map_items, self.unpack_items, self.log_stats, self.lazy, self.overrides)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@monai/transforms/compose.py` around lines 354 - 361, Compose.flatten() currently builds and returns a new Compose instance with only new_transforms, dropping parent attributes; update Compose.flatten to preserve and pass through the parent's configuration (map_items, unpack_items, log_stats, lazy, overrides) when constructing the returned Compose so the flattened pipeline is equivalent to the original (mirroring OneOf.flatten behavior). Locate the Compose.flatten implementation and change the return from Compose(new_transforms) to Compose(new_transforms, map_items=self.map_items, unpack_items=self.unpack_items, log_stats=self.log_stats, lazy=self.lazy, overrides=self.overrides) or otherwise propagate those attributes from self into the new Compose instance.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@monai/transforms/compose.py`:
- Around line 354-361: Compose.flatten() currently builds and returns a new
Compose instance with only new_transforms, dropping parent attributes; update
Compose.flatten to preserve and pass through the parent's configuration
(map_items, unpack_items, log_stats, lazy, overrides) when constructing the
returned Compose so the flattened pipeline is equivalent to the original
(mirroring OneOf.flatten behavior). Locate the Compose.flatten implementation
and change the return from Compose(new_transforms) to Compose(new_transforms,
map_items=self.map_items, unpack_items=self.unpack_items,
log_stats=self.log_stats, lazy=self.lazy, overrides=self.overrides) or otherwise
propagate those attributes from self into the new Compose instance.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 4cdba609-bf44-46ad-a65c-afd42771ee1d
📒 Files selected for processing (2)
monai/transforms/compose.pytests/transforms/compose/test_compose.py
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/transforms/compose/test_compose.py
|
I'd like to have a careful think about this. There are a few ways in which nested Composes in their current form have problems, and they could do with being addressed holistically. At least, I'd like to make sure it doesn't potentially affect a broader fix. |
atbenmurray
left a comment
atbenmurray
left a comment
There was a problem hiding this comment.
My initial feeling is that this should just work and doesn't get in the way of any future refactors.
The change to flatten is an ancillary fix that you made while changing this, yes? Might be worth expanding the tests around this for outer and inner composes having different combinations of flags, and perhaps two internal composes, one of which has it and one of which doesn't.
aymuos15
force-pushed
the
worktree-fix-nested-compose-map-items
branch
from
e30ac95 to
bf8e78e
Compare
|
@atbenmurray thank you very much for pointing out the expanded tests. Have added them now. |
aymuos15
force-pushed
the
worktree-fix-nested-compose-map-items
branch
from
bf8e78e to
f89ab6f
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/transforms/compose/test_compose.py (1)
796-803: ⚡ Quick winSome new regressions still bypass the
map_itemsbranch.
map_itemsonly matters for list/tuple inputs. These cases use a tensor or scalar, so the pre-fix implementation would still pass. Please drive at least one inverse roundtrip and the three-level nesting case through list/tuple data, and add the same inverse coverage forRandomOrder/SomeOf, since those call sites changed too.As per coding guidelines, "Ensure new or modified definitions will be covered by existing or new unit tests."
Also applies to: 880-900, 902-914
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/transforms/compose/test_compose.py` around lines 796 - 803, The test currently uses tensor/scalar inputs so it misses the map_items branch; update tests (e.g., test_inverse_respects_child_map_items and the other noted blocks) to include at least one inverse roundtrip using list and tuple inputs (e.g., [tensor,...] and (tensor,...)) and a three-level nested Compose (Compose inside Compose inside Compose) where the inner Compose has map_items=False to ensure delegation to child Compose.inverse; additionally add equivalent inverse roundtrip tests for RandomOrder and SomeOf compositions (with nested map_items=False children) to exercise the changed call sites and ensure list/tuple paths are covered.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@monai/transforms/compose.py`:
- Around line 356-368: The flattening logic currently only checks t.map_items ==
self.map_items before inlining a nested Compose, which can change behavior if
t.unpack_items differs; update the condition used where Compose instances are
inlined (the block that calls t.flatten().transforms) to require both
t.map_items == self.map_items and t.unpack_items == self.unpack_items so you
only flatten when the parent's and child's unpack_items policies match; keep
using the same Compose.flatten() call and preserve other kwargs (log_stats,
lazy, overrides) as before.
---
Nitpick comments:
In `@tests/transforms/compose/test_compose.py`:
- Around line 796-803: The test currently uses tensor/scalar inputs so it misses
the map_items branch; update tests (e.g., test_inverse_respects_child_map_items
and the other noted blocks) to include at least one inverse roundtrip using list
and tuple inputs (e.g., [tensor,...] and (tensor,...)) and a three-level nested
Compose (Compose inside Compose inside Compose) where the inner Compose has
map_items=False to ensure delegation to child Compose.inverse; additionally add
equivalent inverse roundtrip tests for RandomOrder and SomeOf compositions (with
nested map_items=False children) to exercise the changed call sites and ensure
list/tuple paths are covered.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 391fe130-c76e-4df0-aa45-dfa77ececb7e
📒 Files selected for processing (2)
monai/transforms/compose.pytests/transforms/compose/test_compose.py
Expand test coverage for nested Compose transforms with different map_items configurations, and forward map_items, unpack_items, log_stats, lazy, and overrides through Compose.flatten() so the flattened pipeline is equivalent to the original (mirroring OneOf.flatten behavior). Signed-off-by: Soumya Snigdha Kundu <soumya_snigdha.kundu@kcl.ac.uk>
aymuos15
force-pushed
the
worktree-fix-nested-compose-map-items
branch
from
f89ab6f to
9b94999
Compare
2 participants
Summary
Fixes #7932, #7565
When a child
Composehas a differentmap_itemssetting than its parent, the parent'sapply_transformwould expand list/tuple data before the child ever sees it — silently overriding the child'smap_items.This PR makes three coordinated changes so the child's
map_itemsis respected:apply_transform): Skip list expansion when the transform is aComposeinstance, letting it handle expansion via its ownmap_itemsinexecute_compose._inverse_onehelper): Delegate directly toCompose.inverse()for nestedComposeobjects (includingRandomOrderandSomeOf) instead of routing throughapply_transform(t.inverse, ...).flatten(): Only inline nestedComposeobjects that share the samemap_itemsas the parent. Children with a differentmap_itemsare preserved as-is.Test plan
test_child_map_items_false_receives_list— parentmap_items=True, childmap_items=False: child receives list as-istest_inverse_respects_child_map_items— inverse roundtrip with nested Composetest_parent_no_map_child_map— parentmap_items=False, childmap_items=True: child maps over itemstest_flatten_preserves_different_map_items—flatten()does not merge children with differentmap_items