added list extend to MultiSampleTrait #8531
Conversation
lukas-folle-snkeos
requested review from
KumoLiu,
ericspod and
Nic-Ma
as code owners
WalkthroughThe PR modifies apply_transform in monai/transforms/transform.py to replace a list-comprehension recursion over list/tuple inputs with an explicit loop that accumulates results. At the leaf recursion level, if a transform returns a list/tuple the code now either extends or appends that result depending on whether the original item was a non-list/tuple or a list/tuple, changing flattening behavior. MapTransform.key_iterator's multiline KeyError message was collapsed into a single formatted string. Tests were added in tests/transforms/compose/test_compose.py to verify multi-sample spatial crop behaviors. Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Assessment against linked issues
Assessment against linked issues: Out-of-scope changesNo out-of-scope changes detected. Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Knowledge base: Disabled due to 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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)
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. 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 |
I, Lukas Folle <[email protected]>, hereby add my Signed-off-by to this commit: e0cda55 Signed-off-by: Lukas Folle <[email protected]>
I, Lukas Folle <[email protected]>, hereby add my Signed-off-by to this commit: e0cda55 Signed-off-by: Lukas Folle <[email protected]>
I, Lukas Folle <[email protected]>, hereby add my Signed-off-by to this commit: e0cda55 Signed-off-by: Lukas Folle <[email protected]>
I, Lukas Folle <[email protected]>, hereby add my Signed-off-by to this commit: e0cda55 Signed-off-by: Lukas Folle <[email protected]>
DCO Remediation Commit for Lukas Folle <[email protected]> I, Lukas Folle <[email protected]>, hereby add my Signed-off-by to this commit: eeb7e12 Signed-off-by: Lukas Folle <[email protected]>
![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 (5)
monai/transforms/transform.py (5)
127-137: Docstring: fix parameter name and clarify behavior“map_data” should be “map_items”, and mention that list/tuple returns are flattened one level.
- If `data` is a list or tuple and `map_data` is True, each item of `data` will be transformed + If `data` is a list or tuple and `map_items` is True, each item of `data` will be transformed and this method returns a list of outcomes. @@ - - If an integer is provided, it specifies the maximum level of nesting to which the transformation - should be recursively applied. This allows treating multi-sample transforms applied after another - multi-sample transform while controlling how deep the mapping goes. + - If an integer is provided, it specifies the maximum level of nesting to which the transformation + should be recursively applied. When a transform returns a list or tuple, the results are flattened + one level at each mapped depth.
174-199: Exception logging: style-only — OK; avoid mutatingdatafor loggingFormatting is good. Minor nit: don’t reassign
datawhen logging a first element; use a temp to avoid shadowing.- if isinstance(data, (list, tuple)): - data = data[0] + if isinstance(data, (list, tuple)): + _data_for_log = data[0] + else: + _data_for_log = data @@ - if isinstance(data, dict): - for k, v in data.items(): + if isinstance(_data_for_log, dict): + for k, v in _data_for_log.items(): _log_stats(data=v, prefix=k) else: - _log_stats(data=data) + _log_stats(data=_data_for_log)
459-461: TypeError message should report offending element type, notkeysInside the loop, report
type(key).__name__}for clarity.- raise TypeError( - f"keys must be one of (Hashable, Iterable[Hashable]) but is {type(keys).__name__}." - ) + raise TypeError( + f"keys must be one of (Hashable, Iterable[Hashable]); " + f"found element of type {type(key).__name__}." + )
515-518: key_iterator: signature formatting — OKOptional: annotate as Generator[Any, None, None] for completeness.
66-68: Docstring nits in _apply_transform
- Typo: “this class is function” → “this function”.
- Sphinx ref missing closing backtick: :ref:
Lazy Resampling topic<lazy_resampling>.Also applies to: 75-78
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Knowledge Base: Disabled due to Reviews > Disable Knowledge Base setting
📒 Files selected for processing (1)
monai/transforms/transform.py(10 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/transform.py
⏰ 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: quick-py3 (ubuntu-latest)
- GitHub Check: flake8-py3 (codeformat)
- GitHub Check: quick-py3 (windows-latest)
- GitHub Check: flake8-py3 (pytype)
- GitHub Check: build-docs
- GitHub Check: quick-py3 (macOS-latest)
- GitHub Check: flake8-py3 (mypy)
- GitHub Check: packaging
- GitHub Check: min-dep-os (ubuntu-latest)
- GitHub Check: min-dep-os (macOS-latest)
- GitHub Check: min-dep-pytorch (2.7.1)
- GitHub Check: min-dep-pytorch (2.4.1)
- GitHub Check: min-dep-os (windows-latest)
- GitHub Check: min-dep-py3 (3.10)
- GitHub Check: min-dep-pytorch (2.6.0)
- GitHub Check: min-dep-pytorch (2.5.1)
- GitHub Check: min-dep-py3 (3.9)
- GitHub Check: min-dep-py3 (3.11)
- GitHub Check: min-dep-py3 (3.12)
🔇 Additional comments (8)
monai/transforms/transform.py (8)
93-108: _apply_transform: style-only refactor — OKMultiline call/returns improve readability; logic unchanged.
225-263: set_random_state: seeding cast and MAX_SEED mod — OKCasting to int64 and modding by MAX_SEED is appropriate for NumPy 2.0 compatibility; type checks/readability improved.
256-259: Type check message — OKClear, explicit message; no change requested.
277-279: NotImplemented message — OKConsistent and informative.
335-337: NotImplemented message — OKConsistent and informative.
441-448: Hook attachment formatting — OKNo behavioral change; readability improved.
481-483: sync_meta_info call — OKStyle change only; behavior preserved.
511-513: NotImplemented message — OKConsistent across classes.
I, Lukas Folle <[email protected]>, hereby add my Signed-off-by to this commit: e0cda55 Signed-off-by: Lukas Folle <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
monai/transforms/transform.py (2)
156-165: Type annotation tighteningThe function currently declares
-> list[Any] | ReturnTypebut constructslist[ReturnType]. Prefer narrowing tolist[ReturnType] | ReturnTypefor consistency.Proposed signature (outside this hunk):
def apply_transform(...) -> list[ReturnType] | ReturnType: ...
515-518: Return type hint could be more explicitkey_iterator yields either a Hashable or a tuple when extra iterables are present. Consider tightening the annotation for better tooling.
- ) -> Generator: + ) -> Generator[Hashable | tuple[Hashable, ...], None, None]:
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Knowledge Base: Disabled due to Reviews > Disable Knowledge Base setting
📒 Files selected for processing (1)
monai/transforms/transform.py(10 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/transform.py
⏰ 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). (4)
- GitHub Check: quick-py3 (ubuntu-latest)
- GitHub Check: quick-py3 (windows-latest)
- GitHub Check: quick-py3 (macOS-latest)
- GitHub Check: packaging
🔇 Additional comments (14)
monai/transforms/transform.py (14)
93-96: Refactor-only: OKCall-site reformatting reads cleaner. No functional change.
98-103: Refactor-only: OKTuple-unpack branch formatting is fine. No logic change.
104-108: Refactor-only: OKSingle-arg branch formatting is fine. No logic change.
174-199: Refactor-only: OKError-logging reformatting is clear; logic unchanged.
225-227: Refactor-only: OKSignature reflow is fine; types unchanged.
245-251: Seed normalization: OKModulo against MAX_SEED post np.int64 cast is reasonable for NumPy 2.0 compat.
256-258: Refactor-only: OKTypeError message formatting is fine.
277-279: Refactor-only: OKNotImplementedError reflow is fine.
335-337: Refactor-only: OKAbstract call error message reflow is fine.
441-443: Refactor-only: OKHook attachment formatting is clearer.
447-448: Refactor-only: OKInverse hook formatting is clearer.
459-461: Refactor-only: OKType check error formatting is fine.
481-483: Refactor-only: OKsync_meta_info call formatting is fine.
511-513: Refactor-only: OKNotImplementedError reflow is fine.
DCO Remediation Commit for Lukas Folle <[email protected]> I, Lukas Folle <[email protected]>, hereby add my Signed-off-by to this commit: e0cda55 Signed-off-by: Lukas Folle <[email protected]>
DCO Remediation Commit for Lukas Folle <[email protected]> I, Lukas Folle <[email protected]>, hereby add my Signed-off-by to this commit: e0cda55 Signed-off-by: Lukas Folle <[email protected]>
|
All remaining failing CI runs are not coming from this change. |
monai/transforms/transform.py
Outdated
| # Only extend if we're at the leaf level (map_items_ == 1) and the transform | ||
| # actually returned a list (not preserving nested structure) | ||
| if isinstance(res_item, list) and map_items_ == 1: | ||
| if not isinstance(item, (list, tuple)): |
There was a problem hiding this comment.
Within the containing block res_item will always be a list (line 151) and so this if statement's condition is always false. I'm not sure the logic is what you're expecting it to be here?
There was a problem hiding this comment.
@ericspod I am not sure I understand what you mean. In case res_item is list, item can still be something else.
There was a problem hiding this comment.
Sorry for the delay, I might have not read correctly what was going on here. I think this block 151-157 can be simplified to:
if isinstance(res_item, list) and map_items_ == 1 and not isinstance(item, (list, tuple)):
res.extend(res_item)
else:
res.append(res_item)The result list is extended when transform turns a non-list/tuple item into a list specifically, and appended otherwise. I think what I'm not sure about is under what situation item would be a list/tuple, and even if it was why we'd append in that case. Am I missing something with that?
We should have an added test as well that would catch the case described in the original issue, ideally using existing transforms rather than dummy ones for testing. Thanks!
|
Hi @lukas-folle-snkeos thanks for the contribution! Please check the logic I flagged and we do need tests to check that this behaviour is correct. You can take the mentioned issue as an example of what to check for but I think we need to test this logic thoroughly and more clearly document what's going on in comments. |
|
Taking a look. |
|
Thanks for the review @ericspod! I added a test with a real transform and refactored the code as suggested. |
I, Lukas Folle <[email protected]>, hereby add my Signed-off-by to this commit: 2d58774 Signed-off-by: Lukas Folle <[email protected]>
I, Lukas Folle <[email protected]>, hereby add my Signed-off-by to this commit: 2d58774 Signed-off-by: Lukas Folle <[email protected]>
I, Lukas Folle <[email protected]>, hereby add my Signed-off-by to this commit: 2d58774 Signed-off-by: Lukas Folle <[email protected]> Merge branch 'dev' of github.com:lukas-folle-snkeos/MONAI into dev
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
tests/transforms/compose/test_compose.py (3)
286-290: Use existingmtimport for consistency.Avoid method-local imports; leverage
import monai.transforms as mtalready present.- from monai.transforms import CenterSpatialCrop, RandSpatialCropSamples - - center_crop = CenterSpatialCrop([128, 128]) - multi_sample_transform = RandSpatialCropSamples([64, 64], 1) + center_crop = mt.CenterSpatialCrop([128, 128]) + multi_sample_transform = mt.RandSpatialCropSamples([64, 64], 1)
293-305: Prefer unittest assertions over bareassert.Gives richer failure messages and isn’t optimized away with
-O. Also add explicit tensor-type checks.- assert execute_compose(img, [center_crop]).shape == torch.Size([1, 128, 128]) + self.assertEqual(execute_compose(img, [center_crop]).shape, torch.Size([1, 128, 128])) single_multi_sample_trait_result = execute_compose(img, [multi_sample_transform, center_crop]) - assert ( - isinstance(single_multi_sample_trait_result, list) - and len(single_multi_sample_trait_result) == 1 - and single_multi_sample_trait_result[0].shape == torch.Size([1, 64, 64]) - ) + self.assertIsInstance(single_multi_sample_trait_result, list) + self.assertEqual(len(single_multi_sample_trait_result), 1) + self.assertIsInstance(single_multi_sample_trait_result[0], torch.Tensor) + self.assertEqual(single_multi_sample_trait_result[0].shape, torch.Size([1, 64, 64])) double_multi_sample_trait_result = execute_compose(img, [multi_sample_transform, multi_sample_transform, center_crop]) - assert ( - isinstance(double_multi_sample_trait_result, list) - and len(double_multi_sample_trait_result) == 1 - and double_multi_sample_trait_result[0].shape == torch.Size([1, 64, 64]) - ) + self.assertIsInstance(double_multi_sample_trait_result, list) + self.assertEqual(len(double_multi_sample_trait_result), 1) + self.assertIsInstance(double_multi_sample_trait_result[0], torch.Tensor) + self.assertEqual(double_multi_sample_trait_result[0].shape, torch.Size([1, 64, 64]))
291-306: Add 2 more assertions to harden behavior.
- Cardinality when chaining (e.g., 2 samples then 2 samples => 4).
- Preservation when the original input is a list (no over-flatten).
Proposed test to add nearby:
def test_multi_sample_trait_cardinality_and_preserve_nested(self): img = torch.zeros([1, 128, 128]) t2 = mt.RandSpatialCropSamples([32, 32], num_samples=2) # chaining should multiply counts: 2 x 2 = 4, flattened res = execute_compose(img, [t2, t2]) self.assertIsInstance(res, list) self.assertEqual(len(res), 4) for r in res: self.assertEqual(r.shape, torch.Size([1, 32, 32])) # original list input should preserve nested structure res2 = execute_compose([img], [t2]) self.assertIsInstance(res2, list) self.assertEqual(len(res2), 1) self.assertIsInstance(res2[0], list) self.assertEqual(len(res2[0]), 2)
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (2)
monai/transforms/transform.py(2 hunks)tests/transforms/compose/test_compose.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- monai/transforms/transform.py
🧰 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:
tests/transforms/compose/test_compose.py
⏰ 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: build-docs
- GitHub Check: quick-py3 (ubuntu-latest)
- GitHub Check: quick-py3 (macOS-latest)
- GitHub Check: quick-py3 (windows-latest)
- GitHub Check: flake8-py3 (codeformat)
- GitHub Check: flake8-py3 (mypy)
- GitHub Check: packaging
- GitHub Check: flake8-py3 (pytype)
- GitHub Check: min-dep-pytorch (2.6.0)
- 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.11)
- GitHub Check: min-dep-py3 (3.9)
- GitHub Check: min-dep-py3 (3.10)
- GitHub Check: min-dep-py3 (3.12)
- GitHub Check: min-dep-os (ubuntu-latest)
- GitHub Check: min-dep-os (windows-latest)
- GitHub Check: min-dep-os (macOS-latest)
🔇 Additional comments (1)
tests/transforms/compose/test_compose.py (1)
285-306: Good targeted coverage for MultiSampleTrait flattening.This guards against list-of-lists when chaining multi-sample transforms. Nice.
DCO Remediation Commit for Lukas Folle <[email protected]> I, Lukas Folle <[email protected]>, hereby add my Signed-off-by to this commit: 2d58774 Signed-off-by: Lukas Folle <[email protected]>
|
Hi folks. I've taken an initial look at this. I'm always a little afraid of these kind of changes that mandate a particular behaviour. Are there no situations where someone might multi-sample something and then multi-sample the multi-sampled thing and expect the lists to be nested? Should this be fixed in execute_compose instead? Please give me a couple of hours today to take a look at that. |
Fixes #8528.
Description
A few sentences describing the changes proposed in this pull request.
Types of changes
./runtests.sh -f -u --net --coverage../runtests.sh --quick --unittests --disttests.make htmlcommand in thedocs/folder.