Warn when PydicomReader falls back to an identity affine (#8468)#8934
Open
Shizoqua wants to merge 1 commit into
Open
Warn when PydicomReader falls back to an identity affine (#8468)#8934Shizoqua wants to merge 1 commit into
Shizoqua wants to merge 1 commit into
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthrough
Estimated code review effort: 1 (Trivial) | ~3 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
tests/data/test_pydicom_reader.py (1)
22-40: ⚡ Quick winExpand test coverage for completeness.
Add tests for:
- Only position tag present (00200032 without 00200037) to verify symmetric behavior
- Both tags present to verify no warning when metadata is sufficient
🧪 Suggested additional tests
def test_position_only_warns(self): """Verify UserWarning when only position tag is present.""" reader = PydicomReader() metadata = {"00200032": {"Value": [0, 0, 0]}} # position only with self.assertWarns(UserWarning): affine = reader._get_affine(metadata) np.testing.assert_array_equal(affine, np.eye(4)) def test_both_tags_present_no_warning(self): """Verify no warning when both required tags are present.""" reader = PydicomReader() metadata = { "00200037": {"Value": [1, 0, 0, 0, 1, 0]}, "00200032": {"Value": [0, 0, 0]}, } # Should not warn when both tags present with self.assertRaises(AssertionError): with self.assertWarns(UserWarning): affine = reader._get_affine(metadata)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/data/test_pydicom_reader.py` around lines 22 - 40, The test class TestPydicomReaderAffine needs two additional test methods to complete coverage. Add test_position_only_warns method that instantiates PydicomReader and tests the _get_affine method with metadata containing only the position tag (00200032) to verify it warns and returns identity matrix, mirroring the existing test_partial_orientation_tags_warns behavior. Then add test_both_tags_present_no_warning method that tests _get_affine with both required tags (00200037 and 00200032) present to verify that no UserWarning is raised when metadata is complete.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/data/test_pydicom_reader.py`:
- Around line 24-32: The test method
test_missing_orientation_tags_warns_and_returns_identity is missing a
Google-style docstring. Add a docstring to this method that describes its
purpose, explains what behavior is being tested (that missing orientation tags
trigger a UserWarning and return an identity matrix affine), and clarifies the
regression test context for issue `#8468`. The docstring should be placed
immediately after the method definition line and before any code logic.
- Around line 34-40: The test_partial_orientation_tags_warns method is missing a
Google-style docstring. Add a docstring immediately after the method definition
that describes the test purpose, explains what is being verified (that having
only one of the two required orientation tags results in a UserWarning), and
documents the expected behavior (that the affine matrix returned is the identity
matrix). Follow Google-style docstring conventions with a summary line and
additional details as needed.
---
Nitpick comments:
In `@tests/data/test_pydicom_reader.py`:
- Around line 22-40: The test class TestPydicomReaderAffine needs two additional
test methods to complete coverage. Add test_position_only_warns method that
instantiates PydicomReader and tests the _get_affine method with metadata
containing only the position tag (00200032) to verify it warns and returns
identity matrix, mirroring the existing test_partial_orientation_tags_warns
behavior. Then add test_both_tags_present_no_warning method that tests
_get_affine with both required tags (00200037 and 00200032) present to verify
that no UserWarning is raised when metadata is complete.
🪄 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: ef27f8b3-db81-4769-b9b3-5f336ea7a592
📒 Files selected for processing (2)
monai/data/image_reader.pytests/data/test_pydicom_reader.py
Comment on lines
+24
to
+32
| def test_missing_orientation_tags_warns_and_returns_identity(self): | ||
| # Without ImageOrientationPatient (0020,0037) and ImagePositionPatient | ||
| # (0020,0032) the affine cannot be derived. The reader falls back to the | ||
| # identity matrix; regression test for #8468 ensures this is no longer | ||
| # silent so users know orientation/spacing may be wrong. | ||
| reader = PydicomReader() | ||
| with self.assertWarns(UserWarning): | ||
| affine = reader._get_affine({}) | ||
| np.testing.assert_array_equal(affine, np.eye(4)) |
Contributor
There was a problem hiding this comment.
Add docstring per coding guidelines.
Google-style docstring should describe the test purpose, including what is being verified and expected behavior.
📝 Proposed docstring
def test_missing_orientation_tags_warns_and_returns_identity(self):
+ """Verify UserWarning is emitted when both orientation and position tags are absent.
+
+ The affine should default to identity matrix when metadata is empty.
+ """
# Without ImageOrientationPatient (0020,0037) and ImagePositionPatient🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/data/test_pydicom_reader.py` around lines 24 - 32, The test method
test_missing_orientation_tags_warns_and_returns_identity is missing a
Google-style docstring. Add a docstring to this method that describes its
purpose, explains what behavior is being tested (that missing orientation tags
trigger a UserWarning and return an identity matrix affine), and clarifies the
regression test context for issue `#8468`. The docstring should be placed
immediately after the method definition line and before any code logic.
Source: Coding guidelines
Comment on lines
+34
to
+40
| def test_partial_orientation_tags_warns(self): | ||
| # Only one of the two required tags present is still insufficient. | ||
| reader = PydicomReader() | ||
| metadata = {"00200037": {"Value": [1, 0, 0, 0, 1, 0]}} # orientation only | ||
| with self.assertWarns(UserWarning): | ||
| affine = reader._get_affine(metadata) | ||
| np.testing.assert_array_equal(affine, np.eye(4)) |
Contributor
There was a problem hiding this comment.
Add docstring per coding guidelines.
Google-style docstring should describe the test purpose, including what is being verified and expected behavior.
📝 Proposed docstring
def test_partial_orientation_tags_warns(self):
+ """Verify UserWarning is emitted when only orientation tag is present.
+
+ The affine should default to identity when position tag (0020,0032) is missing.
+ """
# Only one of the two required tags present is still insufficient.📝 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.
Suggested change
| def test_partial_orientation_tags_warns(self): | |
| # Only one of the two required tags present is still insufficient. | |
| reader = PydicomReader() | |
| metadata = {"00200037": {"Value": [1, 0, 0, 0, 1, 0]}} # orientation only | |
| with self.assertWarns(UserWarning): | |
| affine = reader._get_affine(metadata) | |
| np.testing.assert_array_equal(affine, np.eye(4)) | |
| def test_partial_orientation_tags_warns(self): | |
| """Verify UserWarning is emitted when only orientation tag is present. | |
| The affine should default to identity when position tag (0020,0032) is missing. | |
| """ | |
| # Only one of the two required tags present is still insufficient. | |
| reader = PydicomReader() | |
| metadata = {"00200037": {"Value": [1, 0, 0, 0, 1, 0]}} # orientation only | |
| with self.assertWarns(UserWarning): | |
| affine = reader._get_affine(metadata) | |
| np.testing.assert_array_equal(affine, np.eye(4)) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/data/test_pydicom_reader.py` around lines 34 - 40, The
test_partial_orientation_tags_warns method is missing a Google-style docstring.
Add a docstring immediately after the method definition that describes the test
purpose, explains what is being verified (that having only one of the two
required orientation tags results in a UserWarning), and documents the expected
behavior (that the affine matrix returned is the identity matrix). Follow
Google-style docstring conventions with a summary line and additional details as
needed.
Source: Coding guidelines
Shizoqua
force-pushed
the
fix/8468-pydicom-affine-warning
branch
from
71eaf18 to
f83623f
Compare
Contributor
Author
|
Rebased onto the latest upstream/dev. All checks clean. Ready for review. |
Shizoqua
force-pushed
the
fix/8468-pydicom-affine-warning
branch
2 times, most recently
from
8d99438 to
0dff7a0
Compare
PydicomReader._get_affine silently returned an identity matrix when the ImageOrientationPatient (0020,0037) and ImagePositionPatient (0020,0032) tags were absent (e.g. some multi-frame Enhanced DICOM), leaving the image orientation and spacing wrong with no indication. Emit a UserWarning naming the missing tags and recommending ITKReader for such files; the fallback behaviour is otherwise unchanged. Fixes Project-MONAI#8468. Signed-off-by: Lanre Shittu <136805224+Shizoqua@users.noreply.github.com>
Shizoqua
force-pushed
the
fix/8468-pydicom-affine-warning
branch
from
0dff7a0 to
a3e90f3
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Fixes #8468. When
ImageOrientationPatient(0020,0037) andImagePositionPatient(0020,0032) are missing from the metadata (e.g. some multi-frame Enhanced DICOM),PydicomReader._get_affinereturnednp.eye(4)with no indication, so downstream orientation and spacing were silently wrong.This adds a
UserWarningthat names the missing tags, states the affine defaults to identity, and suggestsITKReaderfor such files. The fallback behaviour itself is unchanged — this only surfaces it.Types of changes
Testing
Added
tests/data/test_pydicom_reader.pyasserting that_get_affinewarns (UserWarning) and returns identity when the orientation/position tags are absent or only partially present.