-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Update ['pixdim'] after Spacing transform in meta dict. #8269
base: dev
Are you sure you want to change the base?
Changes from 6 commits
cdb8391
8cc6175
501bc45
c5ebb0d
1aabdae
fe05f59
14b4942
3935ff5
464a99d
b73e733
89d71ac
c593d01
6f217d1
2d703c4
b6dd460
a34a656
e4e089f
8b7463d
5da7c66
8503e34
173c6c1
5250d15
f8fcae3
5639313
8c73c69
0f18a3b
707e102
7bee286
651eac5
ae086d0
9e51576
fc1156a
e6b289b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -535,6 +535,9 @@ def __call__( | |
dtype=dtype, | ||
lazy=lazy_, | ||
) | ||
if isinstance(data_array, MetaTensor) and "pixdim" in data_array.meta: | ||
data_array = cast(MetaTensor, data_array.clone()) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why is it necessary to clone the data array here? This is going to have a cost and I think isn't compatible with lazy resampling. Perhaps this is code that should be There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @ericspod Thank you for your suggestion. We initially considered using
Originally, if we didn't use However, as suggested above, using |
||
data_array.set_pixdim() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It seems that the pixel dimensions (pixdim) are only updated in the Spacing transformation. This is why I previously suggested that we should only retain the Additionally, the use of
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @KumoLiu Thank you for your detailed feedback. Our original thinking was that all related information should be update, including: data['image']['pixdim'] However, after reading your response, we're a bit uncertain: are you suggesting that we don't need to update pixdim in |
||
if self.recompute_affine and isinstance(data_array, MetaTensor): | ||
if lazy_: | ||
raise NotImplementedError("recompute_affine is not supported with lazy evaluation.") | ||
|
Original file line number | Diff line number | Diff line change | ||
---|---|---|---|---|
|
@@ -24,11 +24,13 @@ | |||
import numpy as np | ||||
import torch | ||||
|
||||
import monai.transforms as transforms | ||||
from monai.config import DtypeLike, KeysCollection, SequenceStr | ||||
from monai.config.type_definitions import NdarrayOrTensor | ||||
from monai.data.box_utils import BoxMode, StandardMode | ||||
from monai.data.meta_obj import get_track_meta | ||||
from monai.data.meta_tensor import MetaTensor | ||||
from monai.data.utils import is_supported_format | ||||
from monai.networks.layers.simplelayers import GaussianFilter | ||||
from monai.transforms.croppad.array import CenterSpatialCrop | ||||
from monai.transforms.inverse import InvertibleTransform | ||||
|
@@ -520,6 +522,11 @@ def __call__(self, data: Mapping[Hashable, torch.Tensor], lazy: bool | None = No | |||
output_spatial_shape=output_shape_k if should_match else None, | ||||
lazy=lazy_, | ||||
) | ||||
if isinstance(d[key], MetaTensor) and f"{key}_meta_dict" in d: | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this may need to be more generalized. There are options in LoadImaged to change the default naming convention between a key and the meta_dict for that key. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @atbenmurray Thank you for your suggestion. We plan to implement checking all dictionary keys that start with During implementation, we discovered that For example, if the original key is 'image' and the postfix is changed to 'dict', there would be two keys: 'image' and 'image_dict'. However, after executing In our implementation, we've worked around this by directly updating the existing meta dictionaries, which avoids the creation of additional keys. Would you consider this behavior something that should be addressed in a separate issue, or is our current approach sufficient? We'd appreciate your thoughts on this. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Whilst it isn't directly connected to this item, it does expose a design issue that we ought to find a solution to. Given that there is currently no way for anything downstream of LoadImaged to know what key was used for the metadata dictionary, can we instead add an entry to the metadata dictionary that indicates what key it relates to? |
||||
if "filename_or_obj" in d[key].meta and is_supported_format( | ||||
d[key].meta["filename_or_obj"], ["nii", "nii.gz"] | ||||
): | ||||
d = transforms.sync_meta_info(key, d) | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. May I ask why we need this sync here seems it already been done in the MONAI/monai/transforms/transform.py Line 426 in 996e876
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thank you for your question. In |
||||
if output_shape_k is None: | ||||
output_shape_k = d[key].peek_pending_shape() if isinstance(d[key], MetaTensor) else d[key].shape[1:] | ||||
return d | ||||
|
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.
Does it make sense for this to be a method? If it's only going to be called in one place it's simple code could just be put there. If there's anticipation that this would be called by other things then that's fine.
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.
Thanks for your quick reply.
Originally, we considered other files such as DICOM might use
Spacing
, which could involve the usage and accessmeta_tensor.py
property. Therefore, we decided to define a method.However, after we reevaluating the entire codebase this week, it might be better to modify
data["pixdim"]
directly withinTraceableTransform
instead.