-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Fix Invertd #8651
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: dev
Are you sure you want to change the base?
Fix Invertd #8651
Conversation
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.
WalkthroughThis change implements automatic grouping of nested transforms within Compose instances to enable fine-grained inversion control. When Compose initializes, it assigns a Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ 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 |
87d4873 to
c523e94
Compare
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>
c523e94 to
8961b77
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Signed-off-by: sewon.jeon <sewon.jeon@connecteve.com>
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.