Prevent implicit conversion of metatensor to numpy array #8654
Conversation
Signed-off-by: Davis Vigneault <davis.vigneault@gmail.com>
sudomakeinstall
requested review from
KumoLiu,
Nic-Ma and
ericspod
as code owners
WalkthroughTwo files changed. In monai/data/image_writer.py two identical call sites replaced Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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 |
![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
🧹 Nitpick comments (1)
tests/data/test_itk_writer.py (1)
67-72: Good regression test for MetaTensor preservation; consider using a tensor backendThe test cleanly exercises the MetaTensor +
ITKWriter.set_data_arraypath withchannel_dim=-1andsqueeze_end_dims=Trueand verifies both type and metadata preservation, which is exactly what this PR is fixing. If you want to match typical runtime usage more closely, you could optionally initialise with a torch tensor instead of a NumPy array, for example:- data = MetaTensor(np.arange(48).reshape(3, 4, 4, 1), meta={"test_key": "test_value"}) + data = MetaTensor(torch.arange(48).reshape(3, 4, 4, 1), meta={"test_key": "test_value"})but the current version is functionally fine.
📜 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 (2)
monai/data/image_writer.py(1 hunks)tests/data/test_itk_writer.py(2 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:
tests/data/test_itk_writer.pymonai/data/image_writer.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: flake8-py3 (mypy)
- GitHub Check: flake8-py3 (codeformat)
- GitHub Check: build-docs
- GitHub Check: packaging
- GitHub Check: quick-py3 (windows-latest)
- GitHub Check: flake8-py3 (pytype)
- GitHub Check: quick-py3 (ubuntu-latest)
- GitHub Check: quick-py3 (macOS-latest)
- GitHub Check: min-dep-pytorch (2.7.1)
- GitHub Check: min-dep-py3 (3.11)
- GitHub Check: min-dep-py3 (3.9)
- GitHub Check: min-dep-pytorch (2.6.0)
- GitHub Check: min-dep-pytorch (2.5.1)
- GitHub Check: min-dep-py3 (3.10)
- GitHub Check: min-dep-pytorch (2.8.0)
- GitHub Check: min-dep-os (macOS-latest)
- GitHub Check: min-dep-py3 (3.12)
- GitHub Check: min-dep-os (ubuntu-latest)
- GitHub Check: min-dep-os (windows-latest)
🔇 Additional comments (2)
tests/data/test_itk_writer.py (1)
21-21: MetaTensor import aligns with new test usageImporting
MetaTensorhere is necessary for the new test and matches existing import style; no changes needed.monai/data/image_writer.py (1)
326-328: Squeeze change correctly preserves MetaTensor and other tensor typesSwitching from
np.squeeze(data, -1)todata.squeeze(-1)keepsdatain its native type (includingMetaTensor) while still removing trailing singleton dimensions, avoiding the previous implicit conversion tonp.ndarray. Loop semantics and overallconvert_to_channel_lastbehaviour remain intact, so this looks good and is well covered by the new MetaTensor test.
Signed-off-by: Davis Vigneault <davis.vigneault@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 1
📜 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 (1)
tests/data/test_itk_writer.py(3 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:
tests/data/test_itk_writer.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: build-docs
- GitHub Check: quick-py3 (macOS-latest)
- GitHub Check: quick-py3 (windows-latest)
- GitHub Check: flake8-py3 (codeformat)
- GitHub Check: quick-py3 (ubuntu-latest)
- GitHub Check: flake8-py3 (mypy)
- GitHub Check: packaging
- GitHub Check: flake8-py3 (pytype)
- GitHub Check: min-dep-pytorch (2.6.0)
- GitHub Check: min-dep-pytorch (2.8.0)
- GitHub Check: min-dep-pytorch (2.7.1)
- GitHub Check: min-dep-os (windows-latest)
- GitHub Check: min-dep-os (ubuntu-latest)
- GitHub Check: min-dep-os (macOS-latest)
- GitHub Check: min-dep-py3 (3.12)
- GitHub Check: min-dep-pytorch (2.5.1)
- GitHub Check: min-dep-py3 (3.10)
- GitHub Check: min-dep-py3 (3.9)
- GitHub Check: min-dep-py3 (3.11)
🔇 Additional comments (2)
tests/data/test_itk_writer.py (2)
21-21: LGTM!MetaTensor import is necessary for the new test and follows the existing import pattern.
48-48: Clarify the rationale for changing to torch.arange.The change from
np.arangetotorch.arangeintest_rgbisn't directly related to MetaTensor preservation. Is this intentional alignment with torch usage, or should it remain as NumPy?
| def test_metatensor_preserved(self): | ||
| data = MetaTensor(np.arange(48).reshape(3, 4, 4, 1), meta={"test_key": "test_value"}) | ||
| writer = ITKWriter() | ||
| writer.set_data_array(data, channel_dim=-1, squeeze_end_dims=True) | ||
| self.assertIsInstance(writer.data_obj, MetaTensor) | ||
| self.assertEqual(writer.data_obj.meta.get("test_key"), "test_value") |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add docstring and consider additional test coverage.
Missing docstring violates coding guidelines. Additionally, consider testing more scenarios:
- Different
channel_dimvalues (e.g., 0, 1) squeeze_end_dims=Falsecase- Verifying the squeezed shape
Apply this diff to add a docstring:
def test_metatensor_preserved(self):
+ """Test that MetaTensor and its metadata are preserved when squeeze_end_dims=True."""
data = MetaTensor(np.arange(48).reshape(3, 4, 4, 1), meta={"test_key": "test_value"})Consider expanding the test:
def test_metatensor_preserved(self):
"""Test that MetaTensor and its metadata are preserved when squeeze_end_dims=True."""
data = MetaTensor(np.arange(48).reshape(3, 4, 4, 1), meta={"test_key": "test_value"})
writer = ITKWriter()
writer.set_data_array(data, channel_dim=-1, squeeze_end_dims=True)
self.assertIsInstance(writer.data_obj, MetaTensor)
self.assertEqual(writer.data_obj.meta.get("test_key"), "test_value")
# Verify shape after squeeze
self.assertEqual(writer.data_obj.shape, (3, 4, 4))🤖 Prompt for AI Agents
In tests/data/test_itk_writer.py around lines 67 to 72, add a docstring to the
test_metatensor_preserved method and expand assertions to cover more scenarios:
keep the existing MetaTensor and metadata checks, assert the shape after squeeze
for the squeeze_end_dims=True case, add at least one additional case testing a
different channel_dim (e.g., 0 or 1) and assert expected shape/metadata, and add
a case with squeeze_end_dims=False to verify the shape remains unchanged;
implement these as additional assertions or small helper calls within the same
test (or split into small focused tests) so the test verifies preservation of
MetaTensor, metadata, and correct squeezing behavior for multiple parameter
combinations.
|
FYI, the two checks with errors both failed due to running out of disk space.
These seem to be unrelated to the PR. This PR is ready for review. |
1 participant
Fixes #8619.
Description
As detailed in #8619, the
convert_to_channel_lastmethod of theImageWriterclass implicitly convertsdatafrom aMetaTensorto anndarrayhere. This PR addresses this issue by calling thesqueezemethod ondatarather than usingnp.squeeze. A unit test has been added to test this change.Types of changes
./runtests.sh -f -u --net --coverage../runtests.sh --quick --unittests --disttests.make htmlcommand in thedocs/folder.Notes for reviewers/maintainers:
./runtests.sh -f -u --net --coverage, I'm getting several flake8 linting errors in files not touched by this commit. These errors pre-exist the current PR. They are not addressed using the--autofix flag. Since they seem to be unrelated to this PR, I have left them as is, but also have not checked the box above since the tests did not all pass. Reproducing here for reference:Click here to show errors.
./runtests.sh --quick --unittests --disttests, I'm getting an error relating totests/networks/test_bundle_onnx_export.py. I get the same error when I runpython -m tests.networks.test_onnx_exporton thedevbranch. Since this error seems to be unrelated to this PR, I have left it as is, but also did not check the box above since the tests did not all pass. Reproducing here for reference:Click here to show errors.
make html, I get an error related to indentation inmeta_tensor.py. Since this error seems to be unrelated to this PR, I have left it as is, but also did not check the box above since the documentation did not build. Reproducing here for reference:Click here to show errors.
Thank you to the maintainers for their work on this invaluable project and thank you in advance for considering this PR!