Fix #8442: pass meta_data to EnsureChannelFirst when track_meta is False#8835
Conversation
📝 WalkthroughWalkthroughWhen Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 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. Review rate limit: 7/8 reviews remaining, refill in 7 minutes and 30 seconds.Comment |
![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
🤖 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/io/array.py`:
- Around line 302-306: Add a regression test that verifies disabling meta
tracking with set_track_meta(False) does not break channel-first conversion when
using LoadImage or LoadImaged with ensure_channel_first=True: create a test that
calls set_track_meta(False), runs LoadImage(..., ensure_channel_first=True) (and
a LoadImaged variant) on a sample array, and asserts the output is channel-first
(shape/order) and that no MetaTensor metadata is attached (or meta dict remains
absent) to ensure EnsureChannelFirst still runs on plain tensors; reference
EnsureChannelFirst, LoadImage/LoadImaged, ensure_channel_first, set_track_meta,
and MetaTensor when locating code to test.
🪄 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: 9e5bb3f7-9f63-4fb0-9f3b-14fdd624423e
📒 Files selected for processing (1)
monai/transforms/io/array.py
| if self.ensure_channel_first: | ||
| img = EnsureChannelFirst()(img) | ||
| if not isinstance(img, MetaTensor): | ||
| img = EnsureChannelFirst()(img, meta_dict=meta_data) | ||
| else: | ||
| img = EnsureChannelFirst()(img) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check whether tests already cover disabled meta tracking with ensure_channel_first.
# Expect: At least one test combines set_track_meta(False) with ensure_channel_first=True.
rg -n -C3 'set_track_meta\s*\(\s*False\s*\)' --type py
rg -n -C3 'ensure_channel_first\s*=\s*True' --type pyRepository: Project-MONAI/MONAI
Length of output: 18238
🏁 Script executed:
# Find LoadImage/LoadImaged test files
fd -e py "test_load_image" tests/Repository: Project-MONAI/MONAI
Length of output: 135
🏁 Script executed:
# Search within LoadImage test files for set_track_meta(False) usage
rg -l "class.*LoadImage" tests/ --type pyRepository: Project-MONAI/MONAI
Length of output: 166
🏁 Script executed:
# Check test_load_image.py and test_load_imaged.py for both conditions
for f in tests/transforms/test_load_image.py tests/transforms/test_load_imaged.py; do
echo "=== $f ==="
if [ -f "$f" ]; then
rg -A5 -B5 "ensure_channel_first.*=.*True" "$f" | head -50
echo "---"
rg -A5 -B5 "set_track_meta.*False" "$f" | head -50
fi
doneRepository: Project-MONAI/MONAI
Length of output: 783
🏁 Script executed:
# Read test_load_image.py to check for set_track_meta usage
wc -l tests/transforms/test_load_image.pyRepository: Project-MONAI/MONAI
Length of output: 102
🏁 Script executed:
# Read test_load_imaged.py to check for set_track_meta usage
wc -l tests/transforms/test_load_imaged.pyRepository: Project-MONAI/MONAI
Length of output: 103
🏁 Script executed:
# Check both files for any set_track_meta calls
grep -n "set_track_meta" tests/transforms/test_load_image.py tests/transforms/test_load_imaged.pyRepository: Project-MONAI/MONAI
Length of output: 386
🏁 Script executed:
# Get context around ensure_channel_first usage in both test files
rg -B10 -A10 "ensure_channel_first\s*=\s*True" tests/transforms/test_load_image.pyRepository: Project-MONAI/MONAI
Length of output: 45
🏁 Script executed:
rg -B10 -A10 "ensure_channel_meta_first\s*=\s*True" tests/transforms/test_load_imaged.pyRepository: Project-MONAI/MONAI
Length of output: 45
🏁 Script executed:
# Check lines around set_track_meta in test_load_image.py
sed -n '480,495p' tests/transforms/test_load_image.pyRepository: Project-MONAI/MONAI
Length of output: 734
🏁 Script executed:
# Check lines around set_track_meta in test_load_imaged.py
sed -n '168,185p' tests/transforms/test_load_imaged.pyRepository: Project-MONAI/MONAI
Length of output: 696
🏁 Script executed:
# Search for any ensure_channel_first in test files (case-insensitive)
rg -i "ensure_channel_first" tests/transforms/test_load_image.py tests/transforms/test_load_imaged.pyRepository: Project-MONAI/MONAI
Length of output: 831
🏁 Script executed:
# Find TESTS_META definition in test_load_image.py
rg -B5 -A5 "TESTS_META\s*=" tests/transforms/test_load_image.pyRepository: Project-MONAI/MONAI
Length of output: 347
🏁 Script executed:
# Find TESTS_META definition in test_load_imaged.py
rg -B5 -A5 "TESTS_META\s*=" tests/transforms/test_load_imaged.pyRepository: Project-MONAI/MONAI
Length of output: 444
🏁 Script executed:
# Get the full test_correct test method in test_load_image.py
sed -n '485,510p' tests/transforms/test_load_image.pyRepository: Project-MONAI/MONAI
Length of output: 825
🏁 Script executed:
# Check if TESTS_META includes ensure_channel_first anywhere
rg -A20 "TESTS_META\s*=" tests/transforms/test_load_image.py | grep "ensure_channel_first"Repository: Project-MONAI/MONAI
Length of output: 45
🏁 Script executed:
rg -A20 "TESTS_META\s*=" tests/transforms/test_load_imaged.py | grep "ensure_channel_first"Repository: Project-MONAI/MONAI
Length of output: 45
🏁 Script executed:
# Find all test methods that use ensure_channel_first in test files
rg -B2 "ensure_channel_first" tests/transforms/test_load_image.py tests/transforms/test_load_imaged.py | head -80Repository: Project-MONAI/MONAI
Length of output: 1457
🏁 Script executed:
# Check if any of those tests also call set_track_meta(False)
rg -B20 "ensure_channel_first\s*=\s*True" tests/transforms/test_load_image.py | rg "set_track_meta"Repository: Project-MONAI/MONAI
Length of output: 45
🏁 Script executed:
# Check which test method uses TEST_CASE_14, TEST_CASE_18, TEST_CASE_20
rg "TEST_CASE_14|TEST_CASE_18|TEST_CASE_20" tests/transforms/test_load_image.pyRepository: Project-MONAI/MONAI
Length of output: 337
🏁 Script executed:
# Get the test method that uses these test cases
sed -n '445,475p' tests/transforms/test_load_image.pyRepository: Project-MONAI/MONAI
Length of output: 1501
🏁 Script executed:
# Get context around the test_channel_dim method to see full implementation
sed -n '455,480p' tests/transforms/test_load_image.pyRepository: Project-MONAI/MONAI
Length of output: 1281
🏁 Script executed:
# Verify the _cmp method in test_load_imaged.py is not using set_track_meta(False)
sed -n '97,115p' tests/transforms/test_load_imaged.pyRepository: Project-MONAI/MONAI
Length of output: 931
Add regression test for set_track_meta(False) + ensure_channel_first=True.
The fix is correct, but there is no test coverage for set_track_meta(False) combined with LoadImage(..., ensure_channel_first=True) or LoadImaged(..., ensure_channel_first=True). Existing tests cover meta tracking separately from channel-first ordering, but not together. Add a test case to prevent regression of this specific scenario.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@monai/transforms/io/array.py` around lines 302 - 306, Add a regression test
that verifies disabling meta tracking with set_track_meta(False) does not break
channel-first conversion when using LoadImage or LoadImaged with
ensure_channel_first=True: create a test that calls set_track_meta(False), runs
LoadImage(..., ensure_channel_first=True) (and a LoadImaged variant) on a sample
array, and asserts the output is channel-first (shape/order) and that no
MetaTensor metadata is attached (or meta dict remains absent) to ensure
EnsureChannelFirst still runs on plain tensors; reference EnsureChannelFirst,
LoadImage/LoadImaged, ensure_channel_first, set_track_meta, and MetaTensor when
locating code to test.
|
@ericspod: CI is green and the branch is up to date. Happy to answer any questions if helpful. |
ericspod
left a comment
ericspod
left a comment
There was a problem hiding this comment.
Hi @williams145 thanks for this fix. I think it's correct but we should have a regression test for the class as suggested. I suggested a change, please have a look at that as well.
| if not isinstance(img, MetaTensor): | ||
| img = EnsureChannelFirst()(img, meta_dict=meta_data) | ||
| else: | ||
| img = EnsureChannelFirst()(img) |
There was a problem hiding this comment.
| if not isinstance(img, MetaTensor): | |
| img = EnsureChannelFirst()(img, meta_dict=meta_data) | |
| else: | |
| img = EnsureChannelFirst()(img) | |
| ctrans = EnsureChannelFirst() | |
| img = ctrans(img, meta_dict=None if isinstance(img, MetaTensor) else meta_data) |
I'd like to simplify this and remove the poltergeist at the same time. Is it possible to pass meta_data unconditionally to the call, that is second line becomes img = ctrans(img, meta_dict=meta_data)?
…ck_meta is False When set_track_meta(False) is active, MetaTensor.ensure_torch_and_prune_meta returns a plain tensor. The subsequent EnsureChannelFirst() call then has no metadata source, causing a ValueError. Fix: pass meta_data explicitly when img is not a MetaTensor. Signed-off-by: UGBOMEH OGOCHUKWU WILLIAMS <williamsugbomeh@gmail.com>
…n test Apply review suggestion: pass meta_data unconditionally to EnsureChannelFirst — when img is a MetaTensor, EnsureChannelFirst overrides meta_dict with img.meta internally, so the explicit if/else is not needed. Add regression test covering set_track_meta(False) with ensure_channel_first=True on LoadImage. Signed-off-by: UGBOMEH OGOCHUKWU WILLIAMS <williamsugbomeh@gmail.com>
williams145
force-pushed
the
fix/issue-8442-loadimage-track-meta-ensure-channel-first
branch
from
797cdb7 to
36ded05
Compare
|
Thanks for the review. Verified the simplification works. Looking at Also added a regression test ( CI re-running now. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/transforms/test_load_image.py`:
- Around line 500-508: The test hard-codes set_track_meta(True) in the finally
block which can clobber the suite's prior state; change the test
(test_track_meta_false_ensure_channel_first) to capture the current state at the
start (e.g., prev = get_track_meta()), call set_track_meta(False) for the test,
and then in the finally block restore the original with set_track_meta(prev)
instead of set_track_meta(True); keep using LoadImage(...) and the same
assertions and ensure get_track_meta/set_track_meta symbols are used to locate
the change.
🪄 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: aff8f650-076c-4360-8760-9219eba1c8d4
📒 Files selected for processing (2)
monai/transforms/io/array.pytests/transforms/test_load_image.py
| def test_track_meta_false_ensure_channel_first(self): | ||
| try: | ||
| set_track_meta(False) | ||
| r = LoadImage(image_only=True, ensure_channel_first=True)(self.test_data) | ||
| self.assertTupleEqual(r.shape, (1, 128, 128, 128)) | ||
| self.assertIsInstance(r, torch.Tensor) | ||
| self.assertNotIsInstance(r, MetaTensor) | ||
| finally: | ||
| set_track_meta(True) |
There was a problem hiding this comment.
Restore the previous track_meta state.
set_track_meta(True) hard-codes the cleanup state and can clobber whatever the suite had before this test ran.
♻️ Proposed fix
-from monai.data.meta_obj import set_track_meta
+from monai.data.meta_obj import get_track_meta, set_track_meta
@@
def test_track_meta_false_ensure_channel_first(self):
+ prev_track_meta = get_track_meta()
try:
set_track_meta(False)
r = LoadImage(image_only=True, ensure_channel_first=True)(self.test_data)
self.assertTupleEqual(r.shape, (1, 128, 128, 128))
self.assertIsInstance(r, torch.Tensor)
self.assertNotIsInstance(r, MetaTensor)
finally:
- set_track_meta(True)
+ set_track_meta(prev_track_meta)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def test_track_meta_false_ensure_channel_first(self): | |
| try: | |
| set_track_meta(False) | |
| r = LoadImage(image_only=True, ensure_channel_first=True)(self.test_data) | |
| self.assertTupleEqual(r.shape, (1, 128, 128, 128)) | |
| self.assertIsInstance(r, torch.Tensor) | |
| self.assertNotIsInstance(r, MetaTensor) | |
| finally: | |
| set_track_meta(True) | |
| def test_track_meta_false_ensure_channel_first(self): | |
| prev_track_meta = get_track_meta() | |
| try: | |
| set_track_meta(False) | |
| r = LoadImage(image_only=True, ensure_channel_first=True)(self.test_data) | |
| self.assertTupleEqual(r.shape, (1, 128, 128, 128)) | |
| self.assertIsInstance(r, torch.Tensor) | |
| self.assertNotIsInstance(r, MetaTensor) | |
| finally: | |
| set_track_meta(prev_track_meta) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/transforms/test_load_image.py` around lines 500 - 508, The test
hard-codes set_track_meta(True) in the finally block which can clobber the
suite's prior state; change the test
(test_track_meta_false_ensure_channel_first) to capture the current state at the
start (e.g., prev = get_track_meta()), call set_track_meta(False) for the test,
and then in the finally block restore the original with set_track_meta(prev)
instead of set_track_meta(True); keep using LoadImage(...) and the same
assertions and ensure get_track_meta/set_track_meta symbols are used to locate
the change.
2 participants
Fixes #8442
Description
LoadImagewithensure_channel_first=TrueraisesValueErrorwhenset_track_meta(False)is active.Root cause chain:
set_track_meta(False)causesMetaTensor.ensure_torch_and_prune_meta()to return a plain tensor instead of aMetaTensorEnsureChannelFirst()(img)then has no metadata source (img is not a MetaTensor and nometa_dictis passed)ValueErrorbranch inEnsureChannelFirst.__call__at line 205 ofutility/array.pyFix: pass
meta_dataexplicitly toEnsureChannelFirstwhenimgis not aMetaTensor.EnsureChannelFirst.__call__already accepts ameta_dictparameter;meta_datais always a validated dict at this point inLoadImage.__call__.Types of changes
./runtests.sh --quick --unittests --disttests.make htmlcommand in thedocs/folder.