fix(NrrdReader): handle non-spatial axes in 4-D NRRD files#8895
Open
AlexanderSanin wants to merge 1 commit into
Open
fix(NrrdReader): handle non-spatial axes in 4-D NRRD files#8895AlexanderSanin wants to merge 1 commit into
AlexanderSanin wants to merge 1 commit into
Conversation
Contributor
Author
|
Hey @ericspod @garciadias. Could you, please, have a look at this? |
Contributor
📝 WalkthroughWalkthroughNrrdReader now detects channel axes in 4D NRRD volumes by inspecting the header's Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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. Comment |
4-D NRRD files that carry a non-spatial axis (e.g. kinds: list domain domain domain, as produced by 3D Slicer) write the channel direction as '(none)' in the header. pynrrd represents this as a row of NaN values in the 'space directions' array. The previous _get_affine implementation assumed that every row in 'space directions' is a spatial axis, so it received a (4, 3) direction matrix and tried to assign its (3, 4) transpose into a (4, 3) slice, causing a ValueError. Changes: - NrrdReader._get_affine: filter out all-NaN rows (non-spatial axes) before computing the affine matrix. - NrrdReader.get_data: use the 'kinds' header field to automatically detect the channel axis (anything that is not 'domain' or 'space') and set ORIGINAL_CHANNEL_DIM accordingly; SPATIAL_SHAPE is updated to exclude the channel axis to be consistent with other readers. - NrrdReader._convert_f_to_c_order: also reverse the 'kinds' list when present so that the channel axis index remains correct after F→C reordering. - test_nrrd_reader.py: add TEST_CASE_4D_CHANNEL and test_read_4d_channel to cover the corrected behaviour. Closes Project-MONAI#8653 Signed-off-by: Oleksandr Sanin <alexaaander.sanin@gmail.com>
AlexanderSanin
force-pushed
the
fix/nrrd-reader-4d-channel-affine
branch
from
1b93a34 to
352f64f
Compare
![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
🤖 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 `@monai/data/image_reader.py`:
- Around line 1590-1593: The code currently only filters all-NaN rows from the
direction matrix (variable direction) which breaks when _convert_f_to_c_order
flips axes so the non-spatial axis is an all-NaN column; update the logic to
detect and remove all-NaN columns as well as rows: compute row-valid mask and
column-valid mask using np.all(np.isnan(direction.astype(float)), axis=1) and
axis=0 respectively, then select the non-NaN axes from direction (e.g.,
direction = direction[row_valid, :] or direction = direction[:, col_valid] as
appropriate) so both Fortran-to-C reordered data and the index_order="C" 4-D
channel case produce spatial-only axes before the affine assembly that follows
(refer to variable names direction and valid and the conversion path through
_convert_f_to_c_order).
In `@tests/data/test_nrrd_reader.py`:
- Around line 148-172: Add a C-order variant of the existing
test_read_4d_channel that exercises NrrdReader(index_order="C") so the new
_convert_f_to_c_order logic is covered: duplicate the test setup using
NrrdReader(index_order="C"), call reader.read and reader.get_data, then assert
the returned numpy array shape and dtype, verify the channel axis has been
reordered (original_channel_dim now reflects the new position), check
spatial_shape excludes the channel axis accordingly, and assert the affine
matches the expected 4×4 matrix for the C-order case.
🪄 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: 1f03d047-d1fa-4f41-8765-5c91fd307a80
📒 Files selected for processing (2)
monai/data/image_reader.pytests/data/test_nrrd_reader.py
Comment on lines
+1590
to
+1593
| # pynrrd represents non-spatial axes (e.g. 'list' kind in 4-D NRRD files) as rows | ||
| # where every element is NaN. Filter them out so the affine only encodes spatial axes. | ||
| valid = ~np.all(np.isnan(direction.astype(float)), axis=1) | ||
| direction = direction[valid] |
Contributor
There was a problem hiding this comment.
Handle the C-order NaN axis too.
After _convert_f_to_c_order, a 4x3 space directions array becomes 3x4, so the non-spatial axis is an all-NaN column, not a row. Line 1592 only filters rows, which means index_order="C" still reaches a shape mismatch at Line 1598 for the new 4-D channel case.
Suggested fix
- valid = ~np.all(np.isnan(direction.astype(float)), axis=1)
- direction = direction[valid]
+ direction = np.asarray(direction, dtype=float)
+ row_mask = ~np.all(np.isnan(direction), axis=1)
+ col_mask = ~np.all(np.isnan(direction), axis=0)
+ direction = direction[row_mask][:, col_mask]📝 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
| # pynrrd represents non-spatial axes (e.g. 'list' kind in 4-D NRRD files) as rows | |
| # where every element is NaN. Filter them out so the affine only encodes spatial axes. | |
| valid = ~np.all(np.isnan(direction.astype(float)), axis=1) | |
| direction = direction[valid] | |
| # pynrrd represents non-spatial axes (e.g. 'list' kind in 4-D NRRD files) as rows | |
| # where every element is NaN. Filter them out so the affine only encodes spatial axes. | |
| direction = np.asarray(direction, dtype=float) | |
| row_mask = ~np.all(np.isnan(direction), axis=1) | |
| col_mask = ~np.all(np.isnan(direction), axis=0) | |
| direction = direction[row_mask][:, col_mask] |
🤖 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 `@monai/data/image_reader.py` around lines 1590 - 1593, The code currently only
filters all-NaN rows from the direction matrix (variable direction) which breaks
when _convert_f_to_c_order flips axes so the non-spatial axis is an all-NaN
column; update the logic to detect and remove all-NaN columns as well as rows:
compute row-valid mask and column-valid mask using
np.all(np.isnan(direction.astype(float)), axis=1) and axis=0 respectively, then
select the non-NaN axes from direction (e.g., direction = direction[row_valid,
:] or direction = direction[:, col_valid] as appropriate) so both Fortran-to-C
reordered data and the index_order="C" 4-D channel case produce spatial-only
axes before the affine assembly that follows (refer to variable names direction
and valid and the conversion path through _convert_f_to_c_order).
Comment on lines
+148
to
+172
| @parameterized.expand([TEST_CASE_4D_CHANNEL]) | ||
| def test_read_4d_channel(self, data_shape, filename, dtype, reference_header): | ||
| """4-D NRRD with a 'list' channel axis must not crash in _get_affine and must | ||
| set ORIGINAL_CHANNEL_DIM / spatial_shape correctly.""" | ||
| test_image = np.random.rand(*data_shape).astype(dtype) | ||
| with tempfile.TemporaryDirectory() as tempdir: | ||
| filepath = os.path.join(tempdir, filename) | ||
| nrrd.write(filepath, test_image, header=reference_header) | ||
| reader = NrrdReader() | ||
| image_array, image_header = reader.get_data(reader.read(filepath)) | ||
| self.assertIsInstance(image_array, np.ndarray) | ||
| self.assertEqual(image_array.dtype, dtype) | ||
| self.assertTupleEqual(image_array.shape, data_shape) | ||
| # spatial_shape must exclude the channel axis | ||
| self.assertTupleEqual(tuple(image_header["spatial_shape"]), data_shape[1:]) | ||
| # channel dim 0 must be identified | ||
| self.assertEqual(image_header["original_channel_dim"], 0) | ||
| # affine must be a valid 4×4 matrix (3 spatial dims → 4×4) | ||
| self.assertTupleEqual(image_header["affine"].shape, (4, 4)) | ||
| np.testing.assert_allclose( | ||
| image_header["affine"], | ||
| np.array( | ||
| [[-1.0, 0.0, 0.0, -10.0], [0.0, -2.0, 0.0, -20.0], [0.0, 0.0, 3.0, 30.0], [0.0, 0.0, 0.0, 1.0]] | ||
| ), | ||
| ) |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Add the C-order variant of this regression test.
This only covers the default F-order path, but the PR also changes _convert_f_to_c_order. Please add the same fixture with NrrdReader(index_order="C") and assert the reordered channel axis, spatial_shape, and affine. As per coding guidelines, "Ensure new or modified definitions will be covered by existing or new unit tests."
🧰 Tools
🪛 Ruff (0.15.15)
[warning] 165-165: Comment contains ambiguous × (MULTIPLICATION SIGN). Did you mean x (LATIN SMALL LETTER X)?
(RUF003)
[warning] 165-165: Comment contains ambiguous × (MULTIPLICATION SIGN). Did you mean x (LATIN SMALL LETTER X)?
(RUF003)
🤖 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_nrrd_reader.py` around lines 148 - 172, Add a C-order variant
of the existing test_read_4d_channel that exercises NrrdReader(index_order="C")
so the new _convert_f_to_c_order logic is covered: duplicate the test setup
using NrrdReader(index_order="C"), call reader.read and reader.get_data, then
assert the returned numpy array shape and dtype, verify the channel axis has
been reordered (original_channel_dim now reflects the new position), check
spatial_shape excludes the channel axis accordingly, and assert the affine
matches the expected 4×4 matrix for the C-order case.
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
2 participants
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.
Summary
Fixes #8653 —
NrrdReadercrashes with aValueErrorwhen loading 4-D NRRD files that carry a non-spatial (channel) axis, e.g. files produced by 3D Slicer withkinds: list domain domain domain.Root cause: pynrrd represents a
(none)space direction (for non-spatial axes) as a row ofNaNvalues in thespace directionsarray. The previous_get_affineimplementation assumed every row is spatial, so it received a(4, 3)direction matrix and tried to assign its(3, 4)transpose into a(4, 3)slice — causing a shape mismatchValueError.Changes:
NrrdReader._get_affine: filter all-NaN rows (non-spatial axes) before constructing the affine matrix so only genuine spatial directions are used.NrrdReader.get_data: use thekindsheader field to auto-detect the channel axis (any kind that is notdomainorspace), setORIGINAL_CHANNEL_DIMaccordingly, and exclude the channel axis fromSPATIAL_SHAPE— consistent withNibabelReaderbehaviour.NrrdReader._convert_f_to_c_order: also reversekinds(when present) so the detected channel index stays correct after F→C reordering.tests/data/test_nrrd_reader.py: addTEST_CASE_4D_CHANNEL/test_read_4d_channelcovering the corrected behaviour (no crash, correct affine, correct channel dim, correct spatial shape).Test plan
TestNrrdReadertests pass.test_read_4d_channel_0exercises a(3, 4, 5, 6)NRRD file withkinds: list domain domain domainand verifies:ValueErrorfrom_get_affineaffineis a valid(4, 4)matrix encoding only the 3 spatial axesoriginal_channel_dim == 0spatial_shape == (4, 5, 6)(channel excluded)