Enable global coordinates in spatial crop transforms#8794
Open
emanuilo wants to merge 5 commits intoProject-MONAI:devfrom
Open
Enable global coordinates in spatial crop transforms#8794emanuilo wants to merge 5 commits intoProject-MONAI:devfrom
emanuilo wants to merge 5 commits intoProject-MONAI:devfrom
Conversation
Contributor
|
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:
📝 WalkthroughWalkthroughAdds runtime string-key resolution to SpatialCropd so roi_center/roi_size/roi_start/roi_end may be passed as dictionary keys and resolved per call (supports tensors/ndarrays via flattening and rounding). When string-keyed ROIs are used, SpatialCropd defers creation of the internal SpatialCrop, sets requires_current_data True, reconstructs the cropper each invocation, and provides an inverse that reads cropped-region metadata and uses BorderPad. Introduces dictionary point-coordinate transforms TransformPointsWorldToImaged and TransformPointsImageToWorldd (with D/Dict aliases) and updates exports. Adds unit tests for string-key SpatialCropd and the new point transforms. 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 |
emanuilo
force-pushed
the
feature/global-coords-spatial-crop
branch
from
a900675 to
57b3c0b
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
monai/transforms/croppad/dictionary.py (1)
496-553: Add full Google-style docstrings for new/modified methods.
_resolve_roi_param,requires_current_data,__call__, andinverseneed complete Args/Returns/Raises sections per repo guidance.As per coding guidelines, "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."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@monai/transforms/croppad/dictionary.py` around lines 496 - 553, Add full Google-style docstrings for the methods _resolve_roi_param, requires_current_data (property), __call__, and inverse: for _resolve_roi_param(document the val and d parameters, return type torch.Tensor|sequence|scalar, and raise KeyError when a string key is missing; for requires_current_data state it returns bool indicating whether ROI parameters reference data keys; for __call__(document data: Mapping[Hashable, torch.Tensor], lazy: bool|None, describe behavior of resolving string ROIs via _resolve_roi_param, creation of self.cropper, transformation of keys, and return type dict[Hashable, torch.Tensor]); for inverse(document data: Mapping[Hashable, MetaTensor], describe popping the transform via self.cropper.pop_transform(check=False), using TraceKeys.EXTRA_INFO["cropped"] to build a BorderPad inverse, its return type dict[Hashable, MetaTensor], and any exceptions that may be raised if cropper is missing). Ensure Args, Returns, and Raises sections follow Google-style wording and include types and brief descriptions, and place docstrings directly above each corresponding def/property.
🤖 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/croppad/dictionary.py`:
- Around line 496-510: The _resolve_roi_param function only normalizes tensors
but must also normalize numpy arrays and list-like ROI payloads to a 1-D integer
tensor usable by SpatialCrop.compute_slices; update _resolve_roi_param to detect
numpy.ndarray and list/tuple (including nested shapes like (1,1,D)), convert
them to a torch tensor, flatten, apply torch.round and .to(torch.int64) just
like the existing torch.Tensor branch, and return non-sequence/non-array values
unchanged; reference the _resolve_roi_param staticmethod and the variable name
resolved so you can locate and modify the normalization logic.
- Around line 527-537: Don't assign to self.cropper inside __call__; create a
local variable (e.g., cropper = SpatialCrop(...)) and use it for d[key] =
cropper(d[key], lazy=lazy_) so each call uses its own instance and avoids shared
mutable state; if Cropd.inverse() / check_transforms_match expects to find a
transform ID on the transform instance, replicate the same linking used in
CropForegroundd by attaching the required transform identifier/metadata to the
local cropper or to the returned transform object so inverse matching still
works without mutating self.cropper.
---
Nitpick comments:
In `@monai/transforms/croppad/dictionary.py`:
- Around line 496-553: Add full Google-style docstrings for the methods
_resolve_roi_param, requires_current_data (property), __call__, and inverse: for
_resolve_roi_param(document the val and d parameters, return type
torch.Tensor|sequence|scalar, and raise KeyError when a string key is missing;
for requires_current_data state it returns bool indicating whether ROI
parameters reference data keys; for __call__(document data: Mapping[Hashable,
torch.Tensor], lazy: bool|None, describe behavior of resolving string ROIs via
_resolve_roi_param, creation of self.cropper, transformation of keys, and return
type dict[Hashable, torch.Tensor]); for inverse(document data: Mapping[Hashable,
MetaTensor], describe popping the transform via
self.cropper.pop_transform(check=False), using TraceKeys.EXTRA_INFO["cropped"]
to build a BorderPad inverse, its return type dict[Hashable, MetaTensor], and
any exceptions that may be raised if cropper is missing). Ensure Args, Returns,
and Raises sections follow Google-style wording and include types and brief
descriptions, and place docstrings directly above each corresponding
def/property.
🪄 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: a8fc7322-adcb-400b-810d-c858641ccdfa
📒 Files selected for processing (6)
monai/transforms/__init__.pymonai/transforms/croppad/dictionary.pymonai/transforms/utility/dictionary.pytests/transforms/test_spatial_cropd.pytests/transforms/utility/test_transform_points_image_to_worldd.pytests/transforms/utility/test_transform_points_world_to_imaged.py
✅ Files skipped from review due to trivial changes (2)
- tests/transforms/utility/test_transform_points_image_to_worldd.py
- tests/transforms/utility/test_transform_points_world_to_imaged.py
🚧 Files skipped from review as they are similar to previous changes (2)
- monai/transforms/utility/dictionary.py
- tests/transforms/test_spatial_cropd.py
Comment on lines
+527
to
+537
| # Store on self.cropper so that Cropd.inverse() can find the matching | ||
| # transform ID via check_transforms_match. This mirrors the pattern | ||
| # used by CropForegroundd. | ||
| self.cropper = SpatialCrop( | ||
| roi_center=roi_center, roi_size=roi_size, | ||
| roi_start=roi_start, roi_end=roi_end, | ||
| roi_slices=self._roi_slices, lazy=lazy_, | ||
| ) | ||
| for key in self.key_iterator(d): | ||
| d[key] = self.cropper(d[key], lazy=lazy_) | ||
| return d |
Contributor
There was a problem hiding this comment.
Avoid mutating self.cropper per call in the string-key path.
Reassigning self.cropper inside __call__ introduces shared mutable state and can race under concurrent calls. A local per-call cropper preserves behavior without cross-call interference.
Proposed fix
- # Store on self.cropper so that Cropd.inverse() can find the matching
- # transform ID via check_transforms_match. This mirrors the pattern
- # used by CropForegroundd.
- self.cropper = SpatialCrop(
+ # Build a per-call cropper to avoid shared mutable state.
+ cropper = SpatialCrop(
roi_center=roi_center, roi_size=roi_size,
roi_start=roi_start, roi_end=roi_end,
roi_slices=self._roi_slices, lazy=lazy_,
)
for key in self.key_iterator(d):
- d[key] = self.cropper(d[key], lazy=lazy_)
+ d[key] = cropper(d[key], lazy=lazy_)
return d🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@monai/transforms/croppad/dictionary.py` around lines 527 - 537, Don't assign
to self.cropper inside __call__; create a local variable (e.g., cropper =
SpatialCrop(...)) and use it for d[key] = cropper(d[key], lazy=lazy_) so each
call uses its own instance and avoids shared mutable state; if Cropd.inverse() /
check_transforms_match expects to find a transform ID on the transform instance,
replicate the same linking used in CropForegroundd by attaching the required
transform identifier/metadata to the local cropper or to the returned transform
object so inverse matching still works without mutating self.cropper.
57b3c0b to
de0b762
Compare
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (2)
monai/transforms/croppad/dictionary.py (2)
551-561:⚠️ Potential issue | 🟠 MajorAvoid mutating
self.cropperinside__call__.Line [551] rewrites shared instance state per invocation. Concurrent calls can interfere and produce nondeterministic behavior across keys/calls.
Proposed patch
- self.cropper = SpatialCrop( + cropper = SpatialCrop( roi_center=roi_center, roi_size=roi_size, roi_start=roi_start, roi_end=roi_end, roi_slices=self._roi_slices, lazy=lazy_, ) for key in self.key_iterator(d): - d[key] = self.cropper(d[key], lazy=lazy_) + d[key] = cropper(d[key], lazy=lazy_) return d🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@monai/transforms/croppad/dictionary.py` around lines 551 - 561, The code mutates shared instance state by assigning a new SpatialCrop to self.cropper inside __call__, causing race conditions; instead, instantiate a local cropper variable (e.g., cropper = SpatialCrop(...)) within __call__ and use that local variable when calling cropper(d[key], lazy=lazy_), leaving self.cropper unchanged; update references in the loop that currently call self.cropper to use the new local variable and ensure __call__ does not modify any other instance attributes (keep key_iterator and d[key] usage the same).
521-525:⚠️ Potential issue | 🟡 MinorNormalize list/tuple ROI payloads too.
Line [521] only normalizes
np.ndarray/torch.Tensor. String-key ROI values can still belist/tuple(including nested), which bypasses flatten+round and can produce inconsistent slice inputs.Proposed patch
- if isinstance(resolved, np.ndarray): - resolved = torch.from_numpy(resolved) - if isinstance(resolved, torch.Tensor): + if isinstance(resolved, (np.ndarray, list, tuple)): + resolved = torch.as_tensor(resolved) + if isinstance(resolved, torch.Tensor): resolved = torch.round(resolved.flatten()).to(torch.int64)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@monai/transforms/croppad/dictionary.py` around lines 521 - 525, The normalization currently only handles np.ndarray and torch.Tensor; add handling for list/tuple ROI payloads by converting them to a numeric array/tensor before flatten+round so nested lists/tuples don't bypass normalization — e.g., detect if resolved is instance of (list, tuple), convert to np.asarray(resolved) or torch.tensor(resolved) and then follow the existing branch that rounds, flattens and casts to torch.int64; update the logic around the resolved variable used in the rounding block so list/tuple inputs are normalized identically to np.ndarray/torch.Tensor.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@monai/transforms/croppad/dictionary.py`:
- Around line 551-561: The code mutates shared instance state by assigning a new
SpatialCrop to self.cropper inside __call__, causing race conditions; instead,
instantiate a local cropper variable (e.g., cropper = SpatialCrop(...)) within
__call__ and use that local variable when calling cropper(d[key], lazy=lazy_),
leaving self.cropper unchanged; update references in the loop that currently
call self.cropper to use the new local variable and ensure __call__ does not
modify any other instance attributes (keep key_iterator and d[key] usage the
same).
- Around line 521-525: The normalization currently only handles np.ndarray and
torch.Tensor; add handling for list/tuple ROI payloads by converting them to a
numeric array/tensor before flatten+round so nested lists/tuples don't bypass
normalization — e.g., detect if resolved is instance of (list, tuple), convert
to np.asarray(resolved) or torch.tensor(resolved) and then follow the existing
branch that rounds, flattens and casts to torch.int64; update the logic around
the resolved variable used in the rounding block so list/tuple inputs are
normalized identically to np.ndarray/torch.Tensor.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: feac3231-7835-4c5b-91de-2c2f7d7a907a
📒 Files selected for processing (3)
monai/transforms/croppad/dictionary.pytests/transforms/test_spatial_cropd.pytests/transforms/utility/test_transform_points_image_to_worldd.py
✅ Files skipped from review due to trivial changes (2)
- tests/transforms/utility/test_transform_points_image_to_worldd.py
- tests/transforms/test_spatial_cropd.py
Add convenience transforms for converting points between world and image coordinate spaces, and extend SpatialCropd to accept string dictionary keys for ROI parameters, enabling deferred coordinate resolution at call time. New transforms: - TransformPointsWorldToImaged: world-to-image coordinate conversion - TransformPointsImageToWorldd: image-to-world coordinate conversion SpatialCropd changes: - roi_center, roi_size, roi_start, roi_end now accept string keys - When strings are provided, coordinates are resolved from the data dictionary at __call__ time (zero overhead for existing usage) - Tensors from ApplyTransformToPoints are automatically flattened and rounded to integers - Inverse override with check=False for string-key path to handle recreated cropper identity mismatch Signed-off-by: Emanuilo Jovanovic <emanuilo.jovanovic@smartcat.io> Signed-off-by: Emanuilo Jovanovic <emanuilo.jovanovic@hotmail.com>
Signed-off-by: Emanuilo Jovanovic <emanuilo.jovanovic@hotmail.com>
Signed-off-by: Emanuilo Jovanovic <emanuilo.jovanovic@hotmail.com>
emanuilo
force-pushed
the
feature/global-coords-spatial-crop
branch
from
513af39 to
91f40d0
Compare
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
monai/transforms/croppad/dictionary.py (1)
521-525:⚠️ Potential issue | 🟡 MinorNormalize list/tuple ROI payloads as well.
Line [521]-Line [525] only normalize
np.ndarray/torch.Tensor. Nestedlist/tuplepayloads can bypass flatten+round and produce malformed ROI inputs.Proposed fix
- if isinstance(resolved, np.ndarray): - resolved = torch.from_numpy(resolved) + if isinstance(resolved, (np.ndarray, list, tuple)): + resolved = torch.as_tensor(resolved) if isinstance(resolved, torch.Tensor): resolved = torch.round(resolved.flatten()).to(torch.int64)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@monai/transforms/croppad/dictionary.py` around lines 521 - 525, The code only normalizes np.ndarray/torch.Tensor but skips list/tuple ROI payloads, allowing malformed inputs; update the resolved normalization to also detect list/tuple (e.g., isinstance(resolved, (list, tuple))) and coerce them into a numeric array/tensor before rounding: convert lists/tuples to a numpy array (np.asarray) or directly to a torch.tensor, then apply torch.from_numpy if needed, call torch.round(resolved.flatten()).to(torch.int64) as currently done, and return the normalized tensor; reference the existing variable name resolved and the calls torch.from_numpy, torch.round, flatten, to(torch.int64) when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@monai/transforms/croppad/dictionary.py`:
- Around line 521-525: The code only normalizes np.ndarray/torch.Tensor but
skips list/tuple ROI payloads, allowing malformed inputs; update the resolved
normalization to also detect list/tuple (e.g., isinstance(resolved, (list,
tuple))) and coerce them into a numeric array/tensor before rounding: convert
lists/tuples to a numpy array (np.asarray) or directly to a torch.tensor, then
apply torch.from_numpy if needed, call
torch.round(resolved.flatten()).to(torch.int64) as currently done, and return
the normalized tensor; reference the existing variable name resolved and the
calls torch.from_numpy, torch.round, flatten, to(torch.int64) when making the
change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d913aceb-c5c8-423a-a1d9-062925e61f73
📒 Files selected for processing (6)
monai/transforms/__init__.pymonai/transforms/croppad/dictionary.pymonai/transforms/utility/dictionary.pytests/transforms/test_spatial_cropd.pytests/transforms/utility/test_transform_points_image_to_worldd.pytests/transforms/utility/test_transform_points_world_to_imaged.py
✅ Files skipped from review due to trivial changes (3)
- tests/transforms/utility/test_transform_points_image_to_worldd.py
- tests/transforms/test_spatial_cropd.py
- tests/transforms/utility/test_transform_points_world_to_imaged.py
🚧 Files skipped from review as they are similar to previous changes (1)
- monai/transforms/init.py
Signed-off-by: Emanuilo Jovanovic <emanuilo.jovanovic@hotmail.com>
Signed-off-by: Emanuilo Jovanovic <emanuilo.jovanovic@hotmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
I ran into this while building an injury classification pipeline on MRI. My annotations come in physical coordinates and I kept having to manually convert them to voxel space whenever I changed the preprocessing spacing. Found this issue and saw the design was already agreed on, so I went ahead and implemented it.
Follows the approach proposed by @ericspod in #8206.
Resolves #8206
Changes
TransformPointsWorldToImagedandTransformPointsImageToWorldd— thin subclasses ofApplyTransformToPointsdthat hardcode theinvert_affinedirectionSpatialCropdnow accepts string dictionary keys forroi_center,roi_size,roi_start, androi_end. When a string is passed, the actual values are read from the data dict at call time instead of at initDesign notes
SpatialCropdtakes the original code path — zero overhead for existing usageSpatialCropon each__call__(stored onself.cropper), since slice computation must be deferred until the data dict is available.inverse()is overridden for this path to usepop_transform(check=False), since the cropper identity changes between calls — the actual crop info is read from the MetaTensor's transform stackApplyTransformToPoints(shape(C, N, dims)) get flattened and rounded to int viatorch.round(banker's rounding) to avoid systematic biasTests
TransformPointsWorldToImaged(correctness, equivalence with base class, inverse, error cases)TransformPointsImageToWorlddSpatialCropdstring-key support (start/end, center/size, mixed params, tensor shapes, float rounding, missing keys,requires_current_data, multi-key, inverse)TransformPointsWorldToImaged→SpatialCropdwith world-space ROISpatialCropdtests pass unchangedApplyTransformToPointsdtests pass unchanged