Improve Orientation transform to use the "space" (LPS vs RAS) of a metatensor by default #8473
Conversation
CPBridge
changed the title
Signed-off-by: Chris Bridge <[email protected]>
CPBridge
force-pushed
the
bug/lps_orientation_transform
branch
from
0dbcb85 to
e47dd21
Compare
Signed-off-by: Chris Bridge <[email protected]>
Signed-off-by: Chris Bridge <[email protected]>
|
This is now ready for review |
Signed-off-by: Chris Bridge <[email protected]>
Signed-off-by: Chris Bridge <[email protected]>
CPBridge
changed the title
|
Thanks so much to @CPBridge for putting this PR together so quickly and to everyone who maintains this excellent library. I realize there is limited bandwidth but it would be tremendous if @KumoLiu, @ericspod, @Nic-Ma, or another could review this PR. I worry that this could easily introduce laterality errors as people go from research (nifti, RAS) to production (dicom, LPS), and it remains a blocking issue for my group. Thanks again for everyone's efforts. |
|
Hi @sudomakeinstall, while I would obviously like to see this merged soon too, if this is blocking for you there is a (non-obvious) workaround that you can use on current releases. If you know in advance that all the metatensors going through your pipeline will have LPS affine matrices, you can manually set |
monai/transforms/spatial/array.py
Outdated
| @@ -573,7 +574,9 @@ def __init__( | |||
| as_closest_canonical: if True, load the image as closest to canonical axis format. | |||
| labels: optional, None or sequence of (2,) sequences | |||
| (2,) sequences are labels for (beginning, end) of output axis. | |||
| Defaults to ``(('L', 'R'), ('P', 'A'), ('I', 'S'))``. | |||
| Defaults to using the ``"space"`` attribute of a metatensor, | |||
| where appliable, or (('L', 'R'), ('P', 'A'), ('I', 'S'))`` | |||
There was a problem hiding this comment.
I think this should explicitly say that when the "space" meta value is LPS and this argument is None, the value used will be (("R", "L"), ("A", "P"), ("I", "S")) as the axis labels. If labels is not None, it is used regardless of the "space" meta value. If labels is None and the "space" meta value is not LPS, then the meaning of that should be mentioned here as well.
| @@ -560,7 +561,7 @@ def __init__( | |||
| self, | |||
| axcodes: str | None = None, | |||
| as_closest_canonical: bool = False, | |||
| labels: Sequence[tuple[str, str]] | None = (("L", "R"), ("P", "A"), ("I", "S")), | |||
| labels: Sequence[tuple[str, str]] | None = None, | |||
There was a problem hiding this comment.
As we're changing the default value for this argument, we should use deprecated_arg_default to mark this fact and state what the difference is.
There was a problem hiding this comment.
Thanks, this looks good but we should add replaced="1.6" as an argument,
| @@ -564,7 +564,9 @@ def __init__( | |||
| as_closest_canonical: if True, load the image as closest to canonical axis format. | |||
| labels: optional, None or sequence of (2,) sequences | |||
| (2,) sequences are labels for (beginning, end) of output axis. | |||
| Defaults to ``(('L', 'R'), ('P', 'A'), ('I', 'S'))``. | |||
| Defaults to using the ``"space"`` attribute of a metatensor, | |||
| where appliable, or (('L', 'R'), ('P', 'A'), ('I', 'S'))`` | |||
There was a problem hiding this comment.
Pull Request Overview
This PR improves the Orientation transform to properly handle the coordinate space convention (LPS vs RAS) of MetaTensors by default, fixing issue #8467. Previously, the transform assumed RAS space regardless of the tensor's meta["space"] attribute, causing incorrect behavior for LPS tensors.
Key changes:
- Modified the default
labelsparameter from explicit RAS labels toNone, enabling automatic space detection - Added logic to automatically use appropriate labels based on the MetaTensor's space attribute
- Updated test cases to verify correct behavior for both LPS and RAS space conventions
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| monai/transforms/spatial/array.py | Core logic to detect LPS space and set appropriate labels automatically |
| monai/transforms/spatial/dictionary.py | Updated default parameter and documentation for dictionary wrapper |
| tests/transforms/test_orientation.py | Extended test cases to cover both LPS and RAS conventions |
| tests/transforms/test_orientationd.py | Extended dictionary transform tests for both space conventions |
monai/transforms/spatial/array.py
Outdated
| if isinstance(data_array, MetaTensor): | ||
| affine_np, *_ = convert_data_type(data_array.peek_pending_affine(), np.ndarray) | ||
| affine_ = to_affine_nd(sr, affine_np) | ||
|
|
||
| # Set up "labels" such that LPS tensors are handled correctly by default | ||
| if self.labels is None and SpaceKeys(data_array.meta["space"]) == SpaceKeys.LPS: |
There was a problem hiding this comment.
This code assumes the 'space' key always exists in meta dictionary. Consider adding a check for key existence or using .get() with a default value to prevent KeyError when the space attribute is missing.
| if self.labels is None and SpaceKeys(data_array.meta["space"]) == SpaceKeys.LPS: | |
| space_key = data_array.meta.get("space") | |
| if space_key is None: | |
| raise KeyError("The 'space' key is missing in the 'meta' dictionary of the input data_array.") | |
| if self.labels is None and SpaceKeys(space_key) == SpaceKeys.LPS: |
Copilot uses AI. Check for mistakes.
monai/transforms/spatial/array.py
Outdated
| labels = self.labels | ||
|
|
||
| # Set up "labels" such that LPS tensors are handled correctly by default | ||
| if isinstance(data, MetaTensor) and self.labels is None and SpaceKeys(data.meta["space"]) == SpaceKeys.LPS: |
There was a problem hiding this comment.
Same issue as line 631 - this code assumes the 'space' key exists in meta dictionary. Consider adding a check for key existence or using .get() with a default value to prevent KeyError.
| if isinstance(data, MetaTensor) and self.labels is None and SpaceKeys(data.meta["space"]) == SpaceKeys.LPS: | |
| if isinstance(data, MetaTensor) and self.labels is None and SpaceKeys(data.meta.get("space", None)) == SpaceKeys.LPS: |
Copilot uses AI. Check for mistakes.
| @@ -564,7 +564,9 @@ def __init__( | |||
| as_closest_canonical: if True, load the image as closest to canonical axis format. | |||
| labels: optional, None or sequence of (2,) sequences | |||
| (2,) sequences are labels for (beginning, end) of output axis. | |||
| Defaults to ``(('L', 'R'), ('P', 'A'), ('I', 'S'))``. | |||
| Defaults to using the ``"space"`` attribute of a metatensor, | |||
| where appliable, or (('L', 'R'), ('P', 'A'), ('I', 'S'))`` | |||
There was a problem hiding this comment.
Spelling error: 'appliable' should be 'applicable'.
| where appliable, or (('L', 'R'), ('P', 'A'), ('I', 'S'))`` | |
| where applicable, or (('L', 'R'), ('P', 'A'), ('I', 'S'))`` |
Copilot uses AI. Check for mistakes.
monai/transforms/spatial/array.py
Outdated
| @@ -573,7 +574,9 @@ def __init__( | |||
| as_closest_canonical: if True, load the image as closest to canonical axis format. | |||
| labels: optional, None or sequence of (2,) sequences | |||
| (2,) sequences are labels for (beginning, end) of output axis. | |||
| Defaults to ``(('L', 'R'), ('P', 'A'), ('I', 'S'))``. | |||
| Defaults to using the ``"space"`` attribute of a metatensor, | |||
| where appliable, or (('L', 'R'), ('P', 'A'), ('I', 'S'))`` | |||
There was a problem hiding this comment.
Spelling error: 'appliable' should be 'applicable'.
| where appliable, or (('L', 'R'), ('P', 'A'), ('I', 'S'))`` | |
| where applicable, or (('L', 'R'), ('P', 'A'), ('I', 'S'))`` |
Copilot uses AI. Check for mistakes.
Signed-off-by: Chris Bridge <[email protected]>
WalkthroughThe changes update the default behavior of the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Orientation
participant MetaTensor
User->>Orientation: Call with input (optionally MetaTensor)
Orientation->>MetaTensor: Check for "space" metadata
alt "space" == "LPS"
Orientation->>Orientation: Set labels to (("R","L"),("A","P"),("I","S"))
else
Orientation->>Orientation: Set labels to (("L","R"),("P","A"),("I","S"))
end
Orientation->>Orientation: Apply transform using selected labels
Orientation-->>User: Return transformed tensor
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. 📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 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)
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
|
Thanks @ericspod I have addressed all comments from review |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
tests/transforms/test_orientationd.py (1)
90-100: Add missing LPS variant for canonical test case.The canonical test case only includes the RAS variant (
lps_convention=False). For comprehensive coverage, consider adding the corresponding LPS variant withlps_convention=True.Add the missing test case:
] ) + TESTS.append( + [ + {"keys": ["img", "seg"], "as_closest_canonical": True}, + torch.ones((2, 1, 2, 3)), + torch.eye(4), + (2, 1, 2, 3), + "RAS", # or appropriate expected code for LPS canonical + True, + *device, + ] + )
♻️ Duplicate comments (2)
monai/transforms/spatial/array.py (2)
646-651: Address potential KeyError for missing 'space' key.The code assumes the 'space' key exists in
data_array.metabut doesn't handle the case where it might be missing. This could raise aKeyError.Consider using
.get()method with a default value:- if ( - self.labels is None - and "space" in data_array.meta - and SpaceKeys(data_array.meta["space"]) == SpaceKeys.LPS - ): + if ( + self.labels is None + and SpaceKeys(data_array.meta.get("space")) == SpaceKeys.LPS + ):Note: This assumes
SpaceKeys(None)handles the None case appropriately.
687-693: Address potential KeyError for missing 'space' key in inverse method.Similar to the
__call__method, this code assumes the 'space' key exists indata.metabut doesn't handle the case where it might be missing.Consider using
.get()method with a default value:- if ( - isinstance(data, MetaTensor) - and self.labels is None - and "space" in data.meta - and SpaceKeys(data.meta["space"]) == SpaceKeys.LPS - ): + if ( + isinstance(data, MetaTensor) + and self.labels is None + and SpaceKeys(data.meta.get("space")) == SpaceKeys.LPS + ):
🧹 Nitpick comments (1)
monai/transforms/spatial/dictionary.py (1)
577-584: Fix typo in docstring.The docstring updates comprehensively document the new dynamic behavior based on metadata. However, there's a typo on line 584.
- metadata item (if any) of the intput is ignored. + metadata item (if any) of the input is ignored.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
monai/transforms/spatial/array.py(7 hunks)monai/transforms/spatial/dictionary.py(3 hunks)tests/transforms/test_orientation.py(18 hunks)tests/transforms/test_orientationd.py(5 hunks)
⏰ 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: flake8-py3 (mypy)
- GitHub Check: flake8-py3 (codeformat)
- GitHub Check: quick-py3 (ubuntu-latest)
- GitHub Check: flake8-py3 (pytype)
- GitHub Check: build-docs
- GitHub Check: packaging
- GitHub Check: quick-py3 (macOS-latest)
- GitHub Check: quick-py3 (windows-latest)
- GitHub Check: min-dep-os (ubuntu-latest)
- GitHub Check: min-dep-pytorch (2.5.1)
- GitHub Check: min-dep-pytorch (2.7.1)
- GitHub Check: min-dep-pytorch (2.4.1)
- GitHub Check: min-dep-os (macOS-latest)
- GitHub Check: min-dep-pytorch (2.6.0)
- GitHub Check: min-dep-py3 (3.12)
- GitHub Check: min-dep-py3 (3.9)
- GitHub Check: min-dep-os (windows-latest)
- GitHub Check: min-dep-py3 (3.10)
- GitHub Check: min-dep-py3 (3.11)
🔇 Additional comments (17)
monai/transforms/spatial/array.py (5)
67-67: LGTM! Imports are correctly added.The new imports
SpaceKeysanddeprecated_arg_defaultare properly added and align with the functionality being implemented in theOrientationclass.Also applies to: 79-79
561-569: LGTM! Deprecation decorator properly implemented.The deprecation decorator correctly warns users about the default value change and provides clear information about the new behavior using 'space' metadata.
574-574: LGTM! Parameter default change is consistent.The change from the specific tuple default to
Nonealigns with the deprecation decorator and enables the new dynamic label determination based on metadata.
671-671: LGTM! Correct usage of dynamic labels.The dynamic
labelsvariable is correctly passed tonib.orientations.axcodes2ornt, ensuring the appropriate labels are used based on the space metadata or user override.
695-696: LGTM! Consistent usage of dynamic labels in inverse method.Both
nib.orientations.aff2axcodesand theOrientationconstructor correctly use the dynamically determinedlabels, ensuring consistent behavior in the inverse operation.tests/transforms/test_orientationd.py (5)
24-24: LGTM: Import addition supports new LPS convention testing.The
SpaceKeysimport is correctly added to support the new LPS space metadata functionality in the tests.
60-81: Verify expected codes for 2D test cases.Both the RAS and LPS variants of the 2D test case expect the same code "PLS" (lines 66, 77). This seems inconsistent - shouldn't the expected codes differ between RAS and LPS conventions for the same input transformation?
Please verify that the expected axis codes are correct for both conventions in the 2D test scenarios.
110-119: LGTM: Method signature correctly updated.The addition of the
lps_convention: boolparameter to the test method signature is properly positioned and typed, matching the test case structure.
122-123: LGTM: Metadata creation logic correctly implements LPS space detection.The conditional metadata creation properly sets the space key to
SpaceKeys.LPSwhen testing LPS convention, which aligns with the PR objective of making the transform detect and handle LPS metatensors automatically.
133-134: Verify LPS convention labels are correct.The LPS convention labels
(("R", "L"), ("A", "P"), ("I", "S"))are used for axis code verification. Please confirm these labels correctly represent the LPS (Left-Posterior-Superior) coordinate system as opposed to RAS (Right-Anterior-Superior).According to the PR discussion, users should manually set labels to
(("R", "L"), ("A", "P"), ("I", "S"))for LPS affine matrices, which matches what's implemented here. The logic correctly uses conditional labels for verification.monai/transforms/spatial/dictionary.py (3)
74-74: LGTM: Import statement correctly added for deprecation functionality.The import of
deprecated_arg_defaultis properly placed and necessary to support the deprecation decorator used in theOrientationdclass.
549-557: LGTM: Deprecation decorator properly implemented.The deprecation decorator correctly handles the transition from a fixed default to dynamic behavior. The message clearly explains that the transform will now use the 'space' metadata to determine appropriate axis labels, providing good user guidance.
563-563: LGTM: Parameter default value correctly updated.The change from a fixed tuple default to
Nonealigns with the deprecation decorator and enables the new dynamic behavior based on metadata.tests/transforms/test_orientation.py (4)
15-15: LGTM! Import additions support the enhanced test functionality.The
castimport enables proper type hinting for MetaTensor conversions, andSpaceKeysprovides the necessary constants for LPS/RAS space metadata handling.Also applies to: 25-25
29-236: Excellent test coverage enhancement for both coordinate conventions.The systematic addition of
lps_conventionboolean parameter to all test cases effectively doubles the test coverage, ensuring both RAS and LPS coordinate systems are properly validated. This comprehensive approach aligns perfectly with the PR's objective of supporting dynamic space detection.
248-252: Well-structured inverse test setup.The focused test matrix for inverse transforms properly covers both coordinate conventions with device variations, ensuring comprehensive validation of the inverse functionality.
254-322: We still need to inspect the test definition for INIT_PARAM and TESTS to confirm how labels are passed into Orientation. Please run:grep -R "TESTS =" -n tests/transforms/test_orientation.py sed -n '1,200p' tests/transforms/test_orientation.py
| @@ -545,12 +546,21 @@ class Orientationd(MapTransform, InvertibleTransform, LazyTransform): | |||
|
|
|||
| backend = Orientation.backend | |||
|
|
|||
| @deprecated_arg_default( | |||
There was a problem hiding this comment.
This should also have replaced="1.6".
There was a problem hiding this comment.
I tried this already actually but then monai becomes unimportable, so I'm not really sure how this is supposed to work 🤷:
>>> import monai
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
File "/Users/cpb28/Developer/monai/monai/__init__.py", line 101, in <module>
load_submodules(sys.modules[__name__], False, exclude_pattern=excludes)
File "/Users/cpb28/Developer/monai/monai/utils/module.py", line 187, in load_submodules
mod = import_module(name)
^^^^^^^^^^^^^^^^^^^
File "/Users/cpb28/.pyenv/versions/3.12.1/lib/python3.12/importlib/__init__.py", line 90, in import_module
return _bootstrap._gcd_import(name[level:], package, level)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/Users/cpb28/Developer/monai/monai/apps/__init__.py", line 14, in <module>
from .datasets import CrossValidation, DecathlonDataset, MedNISTDataset, TciaDataset
File "/Users/cpb28/Developer/monai/monai/apps/datasets.py", line 33, in <module>
from monai.data import (
File "/Users/cpb28/Developer/monai/monai/data/__init__.py", line 29, in <module>
from .dataset import (
File "/Users/cpb28/Developer/monai/monai/data/dataset.py", line 39, in <module>
from monai.transforms import Compose, Randomizable, RandomizableTrait, Transform, convert_to_contiguous, reset_ops_id
File "/Users/cpb28/Developer/monai/monai/transforms/__init__.py", line 241, in <module>
from .io.array import SUPPORTED_READERS, LoadImage, SaveImage, WriteFileMapping
File "/Users/cpb28/Developer/monai/monai/transforms/io/array.py", line 31, in <module>
from monai.data import image_writer
File "/Users/cpb28/Developer/monai/monai/data/image_writer.py", line 23, in <module>
from monai.transforms.spatial.array import Resize, SpatialResample
File "/Users/cpb28/Developer/monai/monai/transforms/spatial/array.py", line 551, in <module>
class Orientation(InvertibleTransform, LazyTransform):
File "/Users/cpb28/Developer/monai/monai/transforms/spatial/array.py", line 561, in Orientation
@deprecated_arg_default(
^^^^^^^^^^^^^^^^^^^^^^^
File "/Users/cpb28/Developer/monai/monai/utils/deprecate_utils.py", line 313, in _decorator
raise ValueError(
ValueError: Argument `labels` was replaced to the new default value `None` before the specified version 1.6.There was a problem hiding this comment.
OK let's leave that off and it'll be something we revisit when looking at the deprecated items before the next release.
There was a problem hiding this comment.
OK so is there anything left to do on this PR?
Fix for #8467
Description
As detailed in #8467, the
Orientationtransform currently always assumes a tensor's affine matrix is in RAS, regardless of themeta["space"]attribute, leading to incorrect performance for LPS metatensors unless thelabelsare explicitly defined by the user (and it is not at all clear that this needs to be done or how it should be done).The code in this PR checks whether the input tensor is a metatensor with its affine defined in LPS space. If so, it adjusts the
labelspassed tonibabel.orientations.axcodes2orntto give the expected behaviour for LPS tensors.The default value of the
labelsparameter of theOrientationtransform (andOrientationD) has changed from(('L', 'R'), ('P', 'A'), ('I', 'S'))toNone. However, since the behaviour ofnibabel.orientations.axcodes2orntwhen passedlabels=Noneis equivalent to when passinglabels=(('L', 'R'), ('P', 'A'), ('I', 'S')), I would not consider this a breaking change.Types of changes
./runtests.sh -f -u --net --coverage../runtests.sh --quick --unittests --disttests.make htmlcommand in thedocs/folder.