-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Update ['pixdim'] after Spacing transform in meta dict. #8269
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?
Conversation
According to the issue, this PR addresses on the meta dictionary `data['pixdim']` of NIfTI images does not update after applying the `spacing` or `spacingd`.
To align with `affine`, we update `data['pixdim']` and keep the original metainfo in `data['original_pixdim']`.
Additionally, this PR also update the metainfo `key_{meta_key_postfix}['pixdim']` in NIfTI images, consistent with `spaced_data_dict['image_meta_dict']['pixdim']` in issue Project-MONAI#6832.
Signed-off-by: Wei_Chuan, Chiang <[email protected]>
Co-authored-by: einsyang723 <[email protected]>
Co-authored-by: IamTingTing <[email protected]>
Signed-off-by: Wei_Chuan, Chiang <[email protected]>
Signed-off-by: Wei_Chuan, Chiang <[email protected]>
Signed-off-by: Wei_Chuan, Chiang <[email protected]>
Signed-off-by: Wei_Chuan, Chiang <[email protected]>
Signed-off-by: Wei_Chuan, Chiang <[email protected]>
KumoLiu
left a comment
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 the contribution! Leave several comments inline.
|
Hi folks. I had a look through the code and the PR, but I need to do a deeper evaluation of how this intersects with lazy resampling and the consistency of behaviour between MetaTensor and _meta_dict modes |
|
In the middle of extending test scenarios. Please bear with me. |
|
@einsyang723 @IamTingTing The current PR is not our final version, and it has remained unchanged for a while. |
Previously, a method was defined in `meta_tensor`, and using `data_array.clone()` introduced additional costs. After reevaluating the code, modifying the `TraceableTransform.track_transform_meta()` method executed by `SpatialResample` makes it much cleaner and more maintainable. Signed-off-by: Wei_Chuan, Chiang <[email protected]> Co-authored-by: einsyang723 <[email protected]> Co-authored-by: IamTingTing <[email protected]>
Signed-off-by: Wei_Chuan, Chiang <[email protected]> Co-authored-by: einsyang723 <[email protected]> Co-authored-by: IamTingTing <[email protected]>
Previously, a method was defined in `meta_tensor`, and using `data_array.clone()` introduced additional costs. After reevaluating the code, modifying the `TraceableTransform.track_transform_meta()` method executed by `SpatialResample` makes it much cleaner and more maintainable. Signed-off-by: Wei_Chuan, Chiang <[email protected]> Co-authored-by: einsyang723 <[email protected]> Co-authored-by: IamTingTing <[email protected]>
Signed-off-by: Wei_Chuan, Chiang <[email protected]> Co-authored-by: einsyang723 <[email protected]> Co-authored-by: IamTingTing <[email protected]>
Signed-off-by: Wei_Chuan, Chiang <[email protected]> Co-authored-by: einsyang723 <[email protected]> Co-authored-by: IamTingTing <[email protected]>
Signed-off-by: Wei_Chuan, Chiang <[email protected]> Co-authored-by: einsyang723 <[email protected]> Co-authored-by: IamTingTing <[email protected]>
Signed-off-by: Wei_Chuan, Chiang <[email protected]> Co-authored-by: einsyang723 <[email protected]> Co-authored-by: IamTingTing <[email protected]>
|
Picking this up again. Will have a review for you all today. |
Signed-off-by: Wei_Chuan, Chiang <[email protected]>
Signed-off-by: Wei_Chuan, Chiang <[email protected]>
Signed-off-by: Wei_Chuan, Chiang <[email protected]>
Signed-off-by: Wei_Chuan, Chiang <[email protected]>
Signed-off-by: Wei_Chuan, Chiang <[email protected]>
Previously, a method was defined in `meta_tensor`, and using `data_array.clone()` introduced additional costs. After reevaluating the code, modifying the `TraceableTransform.track_transform_meta()` method executed by `SpatialResample` makes it much cleaner and more maintainable. Signed-off-by: Wei_Chuan, Chiang <[email protected]> Co-authored-by: einsyang723 <[email protected]> Co-authored-by: IamTingTing <[email protected]>
Signed-off-by: Wei_Chuan, Chiang <[email protected]> Co-authored-by: einsyang723 <[email protected]> Co-authored-by: IamTingTing <[email protected]>
Signed-off-by: Wei_Chuan, Chiang <[email protected]> Co-authored-by: einsyang723 <[email protected]> Co-authored-by: IamTingTing <[email protected]>
Signed-off-by: Wei_Chuan, Chiang <[email protected]> Co-authored-by: einsyang723 <[email protected]> Co-authored-by: IamTingTing <[email protected]>
Signed-off-by: Wei_Chuan, Chiang <[email protected]> Co-authored-by: einsyang723 <[email protected]> Co-authored-by: IamTingTing <[email protected]>
Signed-off-by: Wei_Chuan, Chiang <[email protected]> Co-authored-by: einsyang723 <[email protected]> Co-authored-by: IamTingTing <[email protected]>
Checking all dictionary keys that start with `{key}_` to support custom settings of `meta_keys` and `meta_key_postfix`.
This ensures that no matter how users configure the naming conventions in `LoadImaged`,
we can correctly synchronize metadata from the MetaTensor to the corresponding meta dictionary.
Signed-off-by: Wei_Chuan, Chiang <[email protected]>
Co-authored-by: einsyang723 <[email protected]>
Co-authored-by: IamTingTing <[email protected]>
Signed-off-by: Wei_Chuan, Chiang <[email protected]> Co-authored-by: einsyang723 <[email protected]> Co-authored-by: IamTingTing <[email protected]>
|
@einsyang723 @slicepaste @ericspod @KumoLiu @IamTingTing |
atbenmurray
left a comment
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.
The change made in monai/transforms/spatial/dictionary.py means the issue that I raised is much less relevant now
@atbenmurray Thank you for the approval. Just to confirm our understanding — this PR can be merged first, and the remaining concerns regarding |
Exactly. Yes. This PR is decoupled from PR #8411 now. |
Thanks for the confirmation. Looking forward to the merge! |
ericspod
left a comment
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.
I think everything has been addressed here, @KumoLiu please trigger blossom and we'll integrate this soon. Thanks!
|
Hi, is there any progress on merging this? Perhaps blossom-ci is stuck? |
WalkthroughThe changes add support for tracking original pixel dimension metadata through image transformations. New enum members PIXDIM and ORIGINAL_PIXDIM are introduced to MetaKeys. The image reader now preserves the original pixdim as a defensive copy. The inverse transform synchronizes pixdim with spacing computed from updated affine values. The dictionary spacing transform conditionally propagates metadata for NIfTI-format inputs. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
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.
Greptile Overview
Greptile Summary
Updates NIfTI metadata handling to ensure pixdim field is correctly updated after Spacing transforms, addressing issues #6840 and #6832 where spacing metadata remained stale.
Key changes:
- Added
MetaKeys.PIXDIMandMetaKeys.ORIGINAL_PIXDIMenum constants for consistent metadata key naming - Modified
NibabelReaderto preserve originalpixdimvalues inoriginal_pixdimbefore any transforms (parallels existingoriginal_affinepattern) - Updated
TraceableTransformto recalculate and updatepixdim[1:]based on the new affine matrix after spatial transforms, preservingpixdim[0](qfac orientation flag) - Enhanced
Spacingddictionary transform to propagate updated metadata from transformed MetaTensors to their associated meta dict keys (e.g.,image_meta_dict) for NIfTI files
Potential concerns:
- No new unit tests were added to verify the pixdim update behavior
- The code in
inverse.py:229assumespixdimis a mutable array-like object (slice assignment won't work on tuples) - The
Spacingdmeta dict update only occurs for NIfTI files, relying on filename extension checking
Confidence Score: 4/5
- Safe to merge with minor testing concerns - functionality appears correct but lacks comprehensive test coverage
- The implementation correctly addresses the reported issues and follows existing patterns (e.g.,
original_affine). The logic preserves the NIfTI qfac value and properly updates spatial dimensions. However, the absence of new tests and reliance onpixdimmutability (which should work for numpy arrays from nibabel headers) introduces some uncertainty about edge cases. - monai/transforms/inverse.py:229 - verify pixdim is always mutable; monai/transforms/spatial/dictionary.py:531 - confirm meta_key dict type assumptions hold
Important Files Changed
File Analysis
| Filename | Score | Overview |
|---|---|---|
| monai/utils/enums.py | 5/5 | Added PIXDIM and ORIGINAL_PIXDIM enum values to MetaKeys for metadata tracking |
| monai/data/image_reader.py | 5/5 | Added logic to preserve original pixdim values by storing a copy in ORIGINAL_PIXDIM when loading NIfTI images |
| monai/transforms/inverse.py | 4/5 | Updates pixdim[1:] to match the new spacing derived from the updated affine matrix, preserving pixdim[0] (qfac) |
| monai/transforms/spatial/dictionary.py | 4/5 | Added logic to propagate updated metadata (including pixdim) from transformed data to associated meta dict keys for NIfTI files |
Sequence Diagram
sequenceDiagram
participant User
participant LoadImaged
participant NibabelReader
participant Spacingd
participant TraceableTransform
participant MetaDict
User->>LoadImaged: Load NIfTI image
LoadImaged->>NibabelReader: read(filename)
NibabelReader->>NibabelReader: _get_meta_dict(img)
Note over NibabelReader: Extract header as dict<br/>including pixdim field
NibabelReader->>NibabelReader: Save original_pixdim = copy(pixdim)
NibabelReader-->>LoadImaged: image data + metadata
LoadImaged->>MetaDict: Store image_meta_dict
Note over MetaDict: Contains pixdim and<br/>original_pixdim
LoadImaged-->>User: data dict with MetaTensor
User->>Spacingd: Apply spacing transform
Spacingd->>TraceableTransform: Process with new spacing
TraceableTransform->>TraceableTransform: Update affine matrix
TraceableTransform->>TraceableTransform: Calculate spacing from affine
TraceableTransform->>TraceableTransform: Update pixdim[1:4] with new spacing
Note over TraceableTransform: Preserves pixdim[0] (qfac)<br/>Updates only spatial dims
TraceableTransform-->>Spacingd: Transformed MetaTensor
Spacingd->>Spacingd: Check if NIfTI file
alt Is NIfTI format
Spacingd->>MetaDict: Update image_meta_dict with new meta
Note over MetaDict: Propagates updated pixdim<br/>to meta dict keys
end
Spacingd-->>User: Transformed data with updated pixdim
4 files reviewed, no comments
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: 0
🧹 Nitpick comments (1)
monai/transforms/spatial/dictionary.py (1)
525-532: Guard meta propagation to dict-typed keys only.Prevents accidental .update() on non-dicts (e.g., future keys). Functionality unchanged.
Apply:
- if isinstance(d[key], MetaTensor): - meta_keys = [k for k in d.keys() if k is not None and k.startswith(f"{key}_")] - for meta_key in meta_keys: - if "filename_or_obj" in d[key].meta and is_supported_format( - d[key].meta["filename_or_obj"], ["nii", "nii.gz"] - ): - d[meta_key].update(d[key].meta) + if isinstance(d[key], MetaTensor): + fn = d[key].meta.get("filename_or_obj") + if fn is not None and is_supported_format(fn, ["nii", "nii.gz"]): + for mk, mv in d.items(): + if mk and mk.startswith(f"{key}_") and isinstance(mv, dict): + mv.update(d[key].meta)
📜 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 (4)
monai/data/image_reader.py(1 hunks)monai/transforms/inverse.py(2 hunks)monai/transforms/spatial/dictionary.py(2 hunks)monai/utils/enums.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/utils/enums.pymonai/data/image_reader.pymonai/transforms/spatial/dictionary.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: min-dep-py3 (3.12)
- GitHub Check: min-dep-os (ubuntu-latest)
- GitHub Check: min-dep-pytorch (2.5.1)
- GitHub Check: min-dep-py3 (3.11)
- GitHub Check: min-dep-pytorch (2.8.0)
- GitHub Check: min-dep-py3 (3.10)
- GitHub Check: min-dep-os (windows-latest)
- GitHub Check: min-dep-py3 (3.9)
- GitHub Check: min-dep-os (macOS-latest)
- GitHub Check: min-dep-pytorch (2.6.0)
- GitHub Check: min-dep-pytorch (2.7.1)
- GitHub Check: flake8-py3 (mypy)
- GitHub Check: flake8-py3 (pytype)
- GitHub Check: quick-py3 (ubuntu-latest)
- GitHub Check: packaging
- GitHub Check: quick-py3 (windows-latest)
- GitHub Check: build-docs
- GitHub Check: quick-py3 (macOS-latest)
- GitHub Check: flake8-py3 (codeformat)
🔇 Additional comments (5)
monai/data/image_reader.py (1)
1115-1116: Defensive copy of pixdim looks good. Please add a test.Solid, minimal change. Add/extend a NIfTI load test to assert ORIGINAL_PIXDIM is a copy and untouched by later mutations.
monai/utils/enums.py (1)
545-547: MetaKeys additions LGTM.Additive, consistent with downstream usage.
monai/transforms/inverse.py (2)
25-25: Import addition is appropriate.
227-230: No changes needed; review comment is incorrect.The
affine_to_spacingfunction is designed to accept both numpy and torch tensors (viaNdarrayTensorunion type). The implementation explicitly handles torch.Tensor at line 727-728 with a dedicated compute path. Adding.numpy()coercion would contradict the function's design and unnecessarily force type conversion.Likely an incorrect or invalid review comment.
monai/transforms/spatial/dictionary.py (1)
32-32: Import addition is fine.
|
/build |
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.
Greptile Overview
Greptile Summary
This PR updates NIfTI pixdim metadata to reflect spacing changes after applying Spacing/Spacingd transforms, maintaining consistency with how affine is handled.
Key Changes:
- Adds
PIXDIMandORIGINAL_PIXDIMconstants toMetaKeysenum - Preserves original
pixdimasoriginal_pixdimwhen loading NIfTI images (similar tooriginal_affine) - Updates
pixdimin MetaTensor metadata when affine transforms are applied (inTraceableTransform.track_transform_meta) - Synchronizes metadata from MetaTensor to dictionary meta keys for NIfTI files in
Spacingd
Issues Found:
- Missing existence check before calling
d[meta_key].update()in monai/transforms/spatial/dictionary.py:531 could causeKeyErrorif meta_key doesn't exist in dictionary
Confidence Score: 3/5
- Safe to merge with one logic issue that needs fixing
- The PR correctly implements pixdim tracking aligned with affine handling, but the Spacingd implementation lacks a safety check that could cause runtime errors when accessing dictionary keys
- monai/transforms/spatial/dictionary.py - needs check that meta_key exists before calling .update()
Important Files Changed
File Analysis
| Filename | Score | Overview |
|---|---|---|
| monai/utils/enums.py | 5/5 | Added PIXDIM and ORIGINAL_PIXDIM enum constants to MetaKeys |
| monai/data/image_reader.py | 5/5 | Stores original pixdim as original_pixdim when loading NIfTI images, consistent with affine handling |
| monai/transforms/inverse.py | 5/5 | Updates pixdim metadata from affine matrix when transforms modify affine |
| monai/transforms/spatial/dictionary.py | 3/5 | Synchronizes metadata from MetaTensor to dictionary meta keys for NIfTI files, but lacks error handling |
Sequence Diagram
sequenceDiagram
participant User
participant LoadImaged
participant NibabelReader
participant Spacingd
participant Spacing
participant SpatialResample
participant TraceableTransform
participant MetaTensor
User->>LoadImaged: Load NIfTI image
LoadImaged->>NibabelReader: get_data(img)
NibabelReader->>NibabelReader: _get_meta_dict(img)
Note over NibabelReader: Extracts pixdim from header
NibabelReader->>NibabelReader: Save original_pixdim
Note over NibabelReader: header[ORIGINAL_PIXDIM] = copy(header[PIXDIM])
NibabelReader-->>LoadImaged: MetaTensor with meta including pixdim
User->>Spacingd: Apply spacing transform
Spacingd->>Spacing: spacing_transform(d[key])
Spacing->>SpatialResample: Resample with new spacing
SpatialResample->>TraceableTransform: track_transform_meta(affine=xform)
alt lazy=False and affine is not None
TraceableTransform->>TraceableTransform: Update affine matrix
TraceableTransform->>TraceableTransform: Calculate new spacing from affine
Note over TraceableTransform: spacing = affine_to_spacing(affine)
TraceableTransform->>MetaTensor: Update meta[PIXDIM]
Note over MetaTensor: meta[PIXDIM][1:1+len(spacing)] = spacing
end
TraceableTransform-->>Spacing: Updated MetaTensor
Spacing-->>Spacingd: Updated d[key]
alt NIfTI file detected
Spacingd->>Spacingd: Find meta_keys (e.g., image_meta_dict)
Spacingd->>Spacingd: d[meta_key].update(d[key].meta)
Note over Spacingd: Syncs pixdim to image_meta_dict
end
Spacingd-->>User: Updated data dict with new pixdim
4 files reviewed, 1 comment
| if isinstance(d[key], MetaTensor): | ||
| meta_keys = [k for k in d.keys() if k is not None and k.startswith(f"{key}_")] | ||
| for meta_key in meta_keys: | ||
| if "filename_or_obj" in d[key].meta and is_supported_format( | ||
| d[key].meta["filename_or_obj"], ["nii", "nii.gz"] | ||
| ): | ||
| d[meta_key].update(d[key].meta) |
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.
logic: missing check that meta_key exists in dictionary before calling .update()
| if isinstance(d[key], MetaTensor): | |
| meta_keys = [k for k in d.keys() if k is not None and k.startswith(f"{key}_")] | |
| for meta_key in meta_keys: | |
| if "filename_or_obj" in d[key].meta and is_supported_format( | |
| d[key].meta["filename_or_obj"], ["nii", "nii.gz"] | |
| ): | |
| d[meta_key].update(d[key].meta) | |
| if isinstance(d[key], MetaTensor): | |
| meta_keys = [k for k in d.keys() if k is not None and k.startswith(f"{key}_")] | |
| for meta_key in meta_keys: | |
| if meta_key in d and "filename_or_obj" in d[key].meta and is_supported_format( | |
| d[key].meta["filename_or_obj"], ["nii", "nii.gz"] | |
| ): | |
| d[meta_key].update(d[key].meta) |
Fixes #6840
Description
According to the issue, this PR addresses on the meta dictionary
data['pixdim']of NIfTI images does not update after applying thespacingorspacingd. To align withaffine, we updatedata['pixdim']and keep the original metainfo indata['original_pixdim']. Additionally, this PR also update the metainfokey_{meta_key_postfix}['pixdim']in NIfTI images, consistent withspaced_data_dict['image_meta_dict']['pixdim']in issue #6832.Types of changes
./runtests.sh -f -u --net --coverage../runtests.sh --quick --unittests --disttests.make htmlcommand in thedocs/folder.