Conversation
eclipse0922
requested review from
KumoLiu,
Nic-Ma and
ericspod
as code owners
|
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:
📝 WalkthroughWalkthroughCompose now assigns a private Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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 |
eclipse0922
force-pushed
the
fix_invertd
branch
2 times, most recently
from
c523e94 to
8961b77
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
monai/transforms/inverse.py (1)
122-133: Group metadata wiring is correct; consider guarding thezipto catch mismatchesThe added
GROUPhandling looks consistent with the new_groupfield and Invertd’s filtering logic. To make this a bit safer (and address the Ruff B905 hint without depending onzip(strict=...)support), you could assert that the number of keys matchesvalsbefore zipping:- vals = ( - self.__class__.__name__, - id(self), - self.tracing, - self._do_transform if hasattr(self, "_do_transform") else True, - ) - info = dict(zip(self.transform_info_keys(), vals)) + vals = ( + self.__class__.__name__, + id(self), + self.tracing, + self._do_transform if hasattr(self, "_do_transform") else True, + ) + keys = self.transform_info_keys() + if len(keys) != len(vals): + raise ValueError(f"transform_info_keys {keys} and vals {vals} must have the same length.") + info = dict(zip(keys, vals))This keeps behavior identical today but will fail fast if either side is changed inconsistently in the future.
monai/transforms/post/dictionary.py (1)
862-884: Group-based filtering in Invertd looks correct; minor cleanup possibleThe
_filter_transforms_by_grouphelper plus the switch to readingapplied_operationsfromMetaTensorcleanly achieves the goal: Invertd now inverts only transforms whoseGROUPmatchesid(self.transform), and the “no matches → return all” fallback keeps old behavior when grouping isn’t available.Two small nits you might consider:
- Import
TraceKeysat module level for consistency with other files instead of inside the helper:-from monai.utils import PostFix, convert_to_tensor, ensure_tuple, ensure_tuple_rep +from monai.utils import PostFix, TraceKeys, convert_to_tensor, ensure_tuple, ensure_tuple_rep- from monai.utils import TraceKeys - # Get the group ID of the transform (Compose instance) target_group = str(id(self.transform))
- Add a brief note in the docstring (or comment above the fallback) that returning
all_transformsis explicitly for backward compatibility when grouping metadata is missing, just so future readers don’t “simplify” it away.Functionally this looks solid.
Also applies to: 919-925
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (5)
monai/transforms/compose.py(1 hunks)monai/transforms/inverse.py(1 hunks)monai/transforms/post/dictionary.py(2 hunks)monai/utils/enums.py(1 hunks)tests/transforms/inverse/test_invertd.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
⚙️ CodeRabbit configuration file
Review the Python code for quality and correctness. Ensure variable names adhere to PEP8 style guides, are sensible and informative in regards to their function, though permitting simple names for loop and comprehension variables. Ensure routine names are meaningful in regards to their function and use verbs, adjectives, and nouns in a semantically appropriate way. Docstrings should be present for all definition which describe each variable, return value, and raised exception in the appropriate section of the Google-style of docstrings. Examine code for logical error or inconsistencies, and suggest what may be changed to addressed these. Suggest any enhancements for code improving efficiency, maintainability, comprehensibility, and correctness. Ensure new or modified definitions will be covered by existing or new unit tests.
Files:
monai/transforms/inverse.pymonai/transforms/post/dictionary.pymonai/transforms/compose.pymonai/utils/enums.pytests/transforms/inverse/test_invertd.py
🧬 Code graph analysis (4)
monai/transforms/inverse.py (1)
monai/utils/enums.py (1)
TraceKeys(324-337)
monai/transforms/post/dictionary.py (2)
monai/utils/enums.py (2)
TraceKeys(324-337)meta(385-386)monai/data/meta_obj.py (4)
applied_operations(190-194)applied_operations(197-203)meta(177-179)meta(182-187)
monai/transforms/compose.py (1)
monai/transforms/inverse.py (2)
inverse(440-448)TraceableTransform(42-384)
tests/transforms/inverse/test_invertd.py (2)
monai/transforms/compose.py (5)
Compose(123-435)inverse(403-420)inverse(557-578)inverse(652-677)inverse(819-845)monai/transforms/post/dictionary.py (1)
Invertd(772-966)
🪛 Ruff (0.14.6)
monai/transforms/inverse.py
128-128: zip() without an explicit strict= parameter
Add explicit value for parameter strict=
(B905)
monai/transforms/compose.py
301-302: try-except-pass detected, consider logging the exception
(S110)
301-301: Do not catch blind exception: Exception
(BLE001)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (19)
- GitHub Check: min-dep-pytorch (2.7.1)
- GitHub Check: min-dep-pytorch (2.8.0)
- GitHub Check: min-dep-pytorch (2.5.1)
- GitHub Check: min-dep-py3 (3.9)
- GitHub Check: min-dep-pytorch (2.6.0)
- GitHub Check: min-dep-py3 (3.10)
- GitHub Check: min-dep-py3 (3.12)
- GitHub Check: min-dep-py3 (3.11)
- GitHub Check: min-dep-os (windows-latest)
- GitHub Check: min-dep-os (ubuntu-latest)
- GitHub Check: min-dep-os (macOS-latest)
- GitHub Check: build-docs
- GitHub Check: quick-py3 (ubuntu-latest)
- GitHub Check: packaging
- GitHub Check: flake8-py3 (codeformat)
- GitHub Check: flake8-py3 (mypy)
- GitHub Check: quick-py3 (macOS-latest)
- GitHub Check: quick-py3 (windows-latest)
- GitHub Check: flake8-py3 (pytype)
🔇 Additional comments (2)
monai/utils/enums.py (1)
324-338: TraceKeys.GROUP addition is consistentAdding
GROUP = "group"toTraceKeysmatches how grouping metadata is written inget_transform_infoand consumed inInvertd; no issues here.tests/transforms/inverse/test_invertd.py (1)
140-357: Good coverage for group-aware Invertd and Compose behaviorThe new tests exercise exactly the tricky scenarios introduced by group tracking: postprocessing before Invertd, multiple independent pipelines, group isolation on
applied_operations, directCompose.inverse()calls, and mixed Invertd/Compose.inverse usage. This should give solid regression coverage for the new_group/TraceKeys.GROUPlogic.
|
I can take a look at this. |
Please do have a look @atbenmurray, thanks. Sorry @eclipse0922 for the delay but we have a CI issue we need to sort with a patch release anyway. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 261-293: Update the docstrings for the _set_transform_groups
method and its inner helper set_group_recursive to Google-style: add an Args:
section describing self and any parameters (e.g., obj, gid) and their
types/purposes, a Returns: section (e.g., None) and a Raises: section if the
code can raise exceptions (e.g., attribute access errors), and brief description
lines explaining behavior (setting TraceableTransform._group, using visited to
avoid recursion, and skipping Compose instances). Ensure the top-level
_set_transform_groups docstring documents group_id and visited usage and that
set_group_recursive documents parameters obj (transform-like object), gid (group
id string), side effects, and that it returns None.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (2)
monai/transforms/compose.pymonai/transforms/inverse.py
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 302-309: The current set_group_recursive loop skips attributes
named "transform" and "transforms", which omits many nested TraceableTransform
instances; update set_group_recursive (used with TraceableTransform and Compose)
to stop excluding those names, and instead: for each attribute on obj (including
names like "transform", "transforms", "sp_transform", "spacing_transform",
"ornt_transform", etc.), if the attribute is a TraceableTransform call
set_group_recursive recursively; if it is a collection (list/tuple/set) or a
mapping, iterate its elements/values and recurse on any TraceableTransform
entries; keep existing guard to avoid recursing into Compose instances and
preserve skipping magic methods. This ensures nested transforms stored directly
or inside containers get the group id.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (1)
monai/transforms/compose.py
eclipse0922
force-pushed
the
fix_invertd
branch
from
ab5c676 to
6e854a8
Compare
ericspod
left a comment
ericspod
left a comment
There was a problem hiding this comment.
Hi @eclipse0922 sorry for the delay on this one. I have had a look through and I think this solution will work. I'm a little worried about how complex our transform implementation is getting with fixes like these being added on, but this looks like a good solution to the problem. I would like @atbenmurray to have a look as well and I made a few comments.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
monai/transforms/post/dictionary.py (1)
867-868: Document the implicit coupling withCompose's group-ID format.
str(id(self.transform))will silently produce no matches (and thus fall back to the unfiltered list) ifComposeever changes the format it uses to stampTraceKeys.GROUP. A brief inline comment makes the dependency explicit.- # Get the group ID of the transform (Compose instance) - target_group = str(id(self.transform)) + # Must match the format used by Compose._set_transform_groups, which stamps + # each child transform's _group with str(id(self)) during __init__. + target_group = str(id(self.transform))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@monai/transforms/post/dictionary.py` around lines 867 - 868, The code assumes Compose stamps TraceKeys.GROUP using str(id(transform)), so the line target_group = str(id(self.transform)) is implicitly coupled to Compose's group-ID format; add a clear inline comment near this line (and/or above the method) stating that TraceKeys.GROUP must match str(id(transform)) produced by Compose, describing the dependency and warning that if Compose changes its stamping format this matching will fail and cause fallback to the unfiltered list; reference self.transform, target_group and TraceKeys.GROUP in the comment so future maintainers notice the coupling.tests/transforms/inverse/test_invertd.py (3)
141-232:test_invertd_with_postprocessing_transforms,test_invertd_multiple_pipelines, andtest_invertd_multiple_postprocessing_transformslack output-shape assertions.
assertIsNotNone/assertInonly confirm the key exists — they don't prove the inversion was correct. Add shape checks to each, e.g.:# After inversion, spatial dims should match the original image self.assertEqual(post[key].shape[1:], img.shape)
test_invertd_group_isolationalready does this correctly at line 272.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/transforms/inverse/test_invertd.py` around lines 141 - 232, The three tests (test_invertd_with_postprocessing_transforms, test_invertd_multiple_pipelines, test_invertd_multiple_postprocessing_transforms) only check presence of the key but not that inversion restored the original shape; add assertions that compare the inverted tensor's spatial shape to the original image's spatial shape (use post[key] and img to assert shapes match, e.g. compare post[key].shape[1:] or appropriate dims against img.shape) so each test verifies the inversion produced the correct output shape.
260-266: Count assertions are fragile: they implicitly assume onlySpacingd(notEnsureChannelFirstd) is aTraceableTransform.If MONAI ever makes
EnsureChannelFirstdtraceable,groups.count(preprocessing1_group)becomes 2 and the assertion silently breaks. Assert at-least-one instead:- self.assertEqual(groups.count(preprocessing1_group), 1) - self.assertEqual(groups.count(preprocessing2_group), 1) + self.assertGreaterEqual(groups.count(preprocessing1_group), 1) + self.assertGreaterEqual(groups.count(preprocessing2_group), 1)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/transforms/inverse/test_invertd.py` around lines 260 - 266, The test assumes exactly one occurrence of each preprocessing group in pre2[key].applied_operations which is fragile; replace the two exact-count assertions that reference groups.count(preprocessing1_group) and groups.count(preprocessing2_group) with at-least-one checks (e.g., use self.assertGreaterEqual(groups.count(preprocessing1_group), 1) and self.assertGreaterEqual(groups.count(preprocessing2_group), 1) or equivalent) so the test asserts presence without failing if another TraceableTransform (like EnsureChannelFirstd) becomes traceable; locate these in the block that builds groups from pre2[key].applied_operations and where preprocessing1_group and preprocessing2_group are compared.
39-39: ImportLambdadfrom the publicmonai.transformsAPI instead of the internal module.The import should be added to the existing
monai.transformsimport block and the separate internal import removed.from monai.transforms import ( ... + Lambdad, ... ) -from monai.transforms.utility.dictionary import Lambdad🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/transforms/inverse/test_invertd.py` at line 39, Replace the internal-module import of Lambdad with the public API export: remove the separate line importing Lambdad from monai.transforms.utility.dictionary and instead add Lambdad to the existing monai.transforms import block so the test imports Lambdad from monai.transforms (update the import group that already imports other symbols from monai.transforms to include Lambdad and delete the internal import line).
🤖 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/post/dictionary.py`:
- Around line 862-874: The docstring for _filter_transforms_by_group is missing
Google-style Args and Returns sections; update the docstring of the function
_filter_transforms_by_group to include an Args section describing the parameter
all_transforms (list[dict]) and what the function expects/uses (e.g., that it
filters by Compose instance group id via self.transform), and a Returns section
describing the returned list[dict] (filtered transforms or original list for
backward compatibility); mention any important behavior (uses TraceKeys.GROUP to
compare against str(id(self.transform))) and no exceptions are raised.
- Around line 916-918: The else branch reads transform_info directly from
d[InvertibleTransform.trace_key(orig_key)] without applying group filtering, so
replace that direct assignment with a filtered result using the existing
_filter_transforms_by_group helper (i.e., call
self._filter_transforms_by_group(...) on the retrieved transform_info before
using it) while leaving meta_info = d.get(orig_meta_key, {}) unchanged; this
ensures InvertibleTransform.trace_key(orig_key) results are filtered by group
and avoids the legacy ID-mismatch bug.
In `@tests/transforms/inverse/test_invertd.py`:
- Around line 169-176: The current try/except around calling postprocessing(pre)
swallows any RuntimeError that isn't the specific ID-mismatch message and can
hide real test failures; remove the try/except and call post =
postprocessing(pre) directly (leave the subsequent assertions
self.assertIsNotNone(post) and self.assertIn(key, post)) so any exception will
naturally fail the test, or alternatively re-raise unexpected RuntimeError
instead of swallowing it — update the block that invokes postprocessing(pre)
accordingly.
---
Nitpick comments:
In `@monai/transforms/post/dictionary.py`:
- Around line 867-868: The code assumes Compose stamps TraceKeys.GROUP using
str(id(transform)), so the line target_group = str(id(self.transform)) is
implicitly coupled to Compose's group-ID format; add a clear inline comment near
this line (and/or above the method) stating that TraceKeys.GROUP must match
str(id(transform)) produced by Compose, describing the dependency and warning
that if Compose changes its stamping format this matching will fail and cause
fallback to the unfiltered list; reference self.transform, target_group and
TraceKeys.GROUP in the comment so future maintainers notice the coupling.
In `@tests/transforms/inverse/test_invertd.py`:
- Around line 141-232: The three tests
(test_invertd_with_postprocessing_transforms, test_invertd_multiple_pipelines,
test_invertd_multiple_postprocessing_transforms) only check presence of the key
but not that inversion restored the original shape; add assertions that compare
the inverted tensor's spatial shape to the original image's spatial shape (use
post[key] and img to assert shapes match, e.g. compare post[key].shape[1:] or
appropriate dims against img.shape) so each test verifies the inversion produced
the correct output shape.
- Around line 260-266: The test assumes exactly one occurrence of each
preprocessing group in pre2[key].applied_operations which is fragile; replace
the two exact-count assertions that reference groups.count(preprocessing1_group)
and groups.count(preprocessing2_group) with at-least-one checks (e.g., use
self.assertGreaterEqual(groups.count(preprocessing1_group), 1) and
self.assertGreaterEqual(groups.count(preprocessing2_group), 1) or equivalent) so
the test asserts presence without failing if another TraceableTransform (like
EnsureChannelFirstd) becomes traceable; locate these in the block that builds
groups from pre2[key].applied_operations and where preprocessing1_group and
preprocessing2_group are compared.
- Line 39: Replace the internal-module import of Lambdad with the public API
export: remove the separate line importing Lambdad from
monai.transforms.utility.dictionary and instead add Lambdad to the existing
monai.transforms import block so the test imports Lambdad from monai.transforms
(update the import group that already imports other symbols from
monai.transforms to include Lambdad and delete the internal import line).
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (2)
monai/transforms/post/dictionary.pytests/transforms/inverse/test_invertd.py
|
@ericspod As follow-up work, I will open a separate refactor thread to reduce complexity in this area (without changing behavior): centralize inverse transform selection/filtering logic in one shared path for both MetaTensor and trace-key data, |
Addresses an issue where `Invertd` fails when postprocessing contains invertible transforms before `Invertd` is called. The solution uses automatic group tracking: `Compose` assigns its ID to child transforms, allowing `Invertd` to filter and select only the relevant transforms for inversion. This ensures correct inversion when multiple transform pipelines are used or when post-processing steps include invertible transforms. `TraceableTransform` now stores group information. `Invertd` now filters transforms by group, falling back to the original behavior if no group information is present (for backward compatibility). Adds tests to verify the fix and group isolation.
DCO Remediation Commit for sewon.jeon <sewon.jeon@connecteve.com> I, sewon.jeon <sewon.jeon@connecteve.com>, hereby add my Signed-off-by to this commit: d97bdd5 I, sewon.jeon <sewon.jeon@connecteve.com>, hereby add my Signed-off-by to this commit: c523e94 Signed-off-by: sewon.jeon <sewon.jeon@connecteve.com>
Signed-off-by: sewon.jeon <sewon.jeon@connecteve.com>
Extend Compose group tagging to discover traceable transforms wrapped in object attributes and container fields, including common patterns like transform/transforms wrappers. Preserve nested Compose boundaries by setting group IDs only on the nested Compose instance itself, without traversing its internal pipeline. Add a regression test covering wrapped transform attributes and nested container traversal. python -m pip install -U -r requirements-dev.txt
Signed-off-by: sewon jeon <irocks0922@gmail.com>
I, sewon.jeon <irocks0922@gmail.com>, hereby add my Signed-off-by to this commit: 5734cff Signed-off-by: sewon.jeon <irocks0922@gmail.com>
I, sewon.jeon <irocks0922@gmail.com>, hereby add my Signed-off-by to this commit: f6777ab Signed-off-by: sewon.jeon <irocks0922@gmail.com>
eclipse0922
force-pushed
the
fix_invertd
branch
from
a1b9c00 to
5abaa08
Compare
I, sewon.jeon <irocks0922@gmail.com>, hereby add my Signed-off-by to this commit: 95222f3 Signed-off-by: sewon.jeon <irocks0922@gmail.com>
eclipse0922
force-pushed
the
fix_invertd
branch
from
c75b4e9 to
8b31f24
Compare
I, sewon jeon <irocks0922@gmail.com>, hereby add my Signed-off-by to this commit: 8b31f24 Signed-off-by: sewon jeon <irocks0922@gmail.com>
I, sewon.jeon <irocks0922@gmail.com>, hereby add my Signed-off-by to this commit: 95222f3 Signed-off-by: sewon.jeon <irocks0922@gmail.com>
I, sewon.jeon <sewon.jeon@connecteve.com>, hereby add my Signed-off-by to this commit: a607e70 Signed-off-by: sewon.jeon <sewon.jeon@connecteve.com>
Signed-off-by: sewon.jeon <irocks0922@gmail.com>
3 participants
Fixes #8396 .
Description
This PR fixes a bug where
Invertdwould fail with a RuntimeError when postprocessing pipelines contain invertible transforms (such asLambdad) beforeInvertdis called. Root Cause:Invertdcopied the entireapplied_operationslist (including both preprocessing AND postprocessing transforms) before inversion. When the preprocessing pipeline's inverse was called, it would pop transforms from the end and encounter postprocessing transforms first, causing an ID mismatch error.Solution: Implements automatic group tracking infrastructure:
Each
Composeinstance automatically assigns its unique ID as a group identifier to all child transforms (recursively, including deeply nested wrapped transforms)Transform metadata in
applied_operationsnow includes an optionalgroupfieldInvertdautomatically filtersapplied_operationsto only include transforms from the targetComposeinstanceZero API changes - the solution is fully automatic and transparent to users
Backward compatible - gracefully handles transforms without groups
Types of changes
./runtests.sh -f -u --net --coverage../runtests.sh --quick --unittests --disttests.make htmlcommand in thedocs/folder.