fix(NrrdReader): handle non-spatial axes in 4-D NRRD files#8895
fix(NrrdReader): handle non-spatial axes in 4-D NRRD files#8895AlexanderSanin wants to merge 1 commit into
Conversation
|
Hey @ericspod @garciadias. Could you, please, have a look at this? |
📝 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 |
1b93a34 to
352f64f
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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
| # 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] |
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.
| # 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).
| @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]] | ||
| ), | ||
| ) |
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.
352f64f to
5c0b819
Compare
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: Claude <noreply@anthropic.com>
5c0b819 to
f626ab8
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
tests/data/test_nrrd_reader.py (1)
146-168:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftAdd C-order variant to complete test coverage.
The PR modifies
_convert_f_to_c_orderto reversekinds(relevant code snippet 3), but this test only exercises F-order. Per past review and coding guidelines, add a companion test withNrrdReader(index_order="C")that verifies the channel axis position after reordering, confirmsspatial_shapestill excludes the channel, and checks the affine remains valid.As per coding guidelines, "Ensure new or modified definitions will be covered by existing or new unit tests."
🤖 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 146 - 168, Add a C-order companion to test_read_4d_channel that constructs the same 4-D test data but instantiates the reader with NrrdReader(index_order="C"), reads the file and asserts the returned image_array dtype/shape match, that image_header["spatial_shape"] equals data_shape[1:] (channel excluded), that image_header["original_channel_dim"] is 0 (channel identified correctly after reordering), and that image_header["affine"] is a 4×4 matrix and equals the expected affine; this will exercise the modified _convert_f_to_c_order path and ensure channel reordering and affine handling are covered.
🧹 Nitpick comments (1)
tests/data/test_nrrd_reader.py (1)
163-163: ⚡ Quick winReplace ambiguous multiplication sign.
Comment uses
×(U+00D7) instead of ASCIIx. Ruff flags this as potentially confusing.📝 Proposed fix
- # affine must be a valid 4×4 matrix (3 spatial dims → 4×4) + # affine must be a valid 4x4 matrix (3 spatial dims -> 4x4)🤖 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` at line 163, The comment about the affine matrix uses the Unicode multiplication sign '×' which Ruff flags; update the comment in tests/data/test_nrrd_reader.py (the line mentioning "affine must be a valid 4×4 matrix") to use ASCII 'x' instead (e.g., "4x4") so the comment reads "affine must be a valid 4x4 matrix (3 spatial dims → 4x4)"; keep the same wording and only replace the character.
🤖 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.
Duplicate comments:
In `@tests/data/test_nrrd_reader.py`:
- Around line 146-168: Add a C-order companion to test_read_4d_channel that
constructs the same 4-D test data but instantiates the reader with
NrrdReader(index_order="C"), reads the file and asserts the returned image_array
dtype/shape match, that image_header["spatial_shape"] equals data_shape[1:]
(channel excluded), that image_header["original_channel_dim"] is 0 (channel
identified correctly after reordering), and that image_header["affine"] is a 4×4
matrix and equals the expected affine; this will exercise the modified
_convert_f_to_c_order path and ensure channel reordering and affine handling are
covered.
---
Nitpick comments:
In `@tests/data/test_nrrd_reader.py`:
- Line 163: The comment about the affine matrix uses the Unicode multiplication
sign '×' which Ruff flags; update the comment in tests/data/test_nrrd_reader.py
(the line mentioning "affine must be a valid 4×4 matrix") to use ASCII 'x'
instead (e.g., "4x4") so the comment reads "affine must be a valid 4x4 matrix (3
spatial dims → 4x4)"; keep the same wording and only replace the character.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 358d4330-b937-4129-9f73-e05dfa43be83
📒 Files selected for processing (2)
monai/data/image_reader.pytests/data/test_nrrd_reader.py
🚧 Files skipped from review as they are similar to previous changes (1)
- monai/data/image_reader.py
ericspod
left a comment
ericspod
left a comment
There was a problem hiding this comment.
Hi @AlexanderSanin thanks for looking at this one, I think it's fine for the problem but please look at Coderabbit's comment and the variable naming thing ruff is complaining about. I made some very minor comments.
| # Use the NRRD 'kinds' field to detect non-spatial (channel) axes. | ||
| # Spatial kinds are 'domain' and 'space'; anything else (e.g. 'list', | ||
| # 'vector') marks a channel axis. | ||
| _SPATIAL_KINDS = {"domain", "space"} |
There was a problem hiding this comment.
Perhaps this should be a global value at the top of the file called NRRD_SPATIAL_KINDS with the associated comment.
| "space directions": np.array([[np.nan, np.nan, np.nan], [1.0, 0.0, 0.0], [0.0, 2.0, 0.0], [0.0, 0.0, 3.0]]), | ||
| "space origin": np.array([10.0, 20.0, 30.0]), | ||
| }, | ||
| ] |
There was a problem hiding this comment.
I don't recall if NRRD supports it, but would we be able to test an input with the channel being last in order as well? If supported, can we test 5D inputs that have time as well as channel dimensions?
| # Spatial kinds are 'domain' and 'space'; anything else (e.g. 'list', | ||
| # 'vector') marks a channel axis. | ||
| _SPATIAL_KINDS = {"domain", "space"} | ||
| ch_axes = [idx for idx, k in enumerate(header.get("kinds", [])) if k.lower() not in _SPATIAL_KINDS] |
There was a problem hiding this comment.
The list comprehension k.lower() will crash with AttributeError if a kinds entry is not a string (e.g. from a malformed NRRD header produced by a third-party tool). Adding an isinstance(k, str) guard is cheap and prevents a cryptic crash at runtime.
Suggested change:
| ch_axes = [idx for idx, k in enumerate(header.get("kinds", [])) if k.lower() not in _SPATIAL_KINDS] | |
| ch_axes = [ | |
| idx | |
| for idx, k in enumerate(header.get("kinds", [])) | |
| if isinstance(k, str) and k.lower() not in NRRD_SPATIAL_KINDS | |
| ] |
4 participants
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)