ENH: Update ConvertImage4DTo3D to support vector 3D and 4D#51
Open
aylward wants to merge 1 commit into
Open
Conversation
Use ITK reader by default, except for NRRD which stores 4D data as images of vectors which is not available in ITK's python wrapping.
WalkthroughThis PR replaces the NRRD-specific Changes4D-to-3D Converter Migration
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 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 |
There was a problem hiding this comment.
Pull request overview
Renames ConvertNRRD4DTo3D → ConvertImage4DTo3D and broadens it to use ITK readers by default for 4D inputs (NIfTI, MHA, …), keeping the pynrrd path only for .nrrd/.seq.nrrd files where ITK Python lacks the needed wrapped vector pixel sizes. Tests, workflows, experiments, docs and CI references are updated to follow the rename.
Changes:
- New
convert_image_4d_to_3d.pywith format-dispatched loader (NRRD → pynrrd, others →itk.imread) and 3D origin/spacing/direction extraction. - Old
convert_nrrd_4d_to_3d.pyremoved; workflow now reads multi-file 3D inputs directly withitk.imreadinstead of viaload_nrrd_3d. - Package re-exports, CLI entry point, tests, experiments, and documentation renamed/updated.
Reviewed changes
Copilot reviewed 24 out of 24 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/physiomotion4d/convert_image_4d_to_3d.py | New ITK-based 4D→3D converter (replaces the NRRD-only one). |
| src/physiomotion4d/convert_nrrd_4d_to_3d.py | Old module removed. |
| src/physiomotion4d/init.py | Re-exports ConvertImage4DTo3D instead of ConvertNRRD4DTo3D. |
| src/physiomotion4d/cli/init.py | Adds convert_image_4d_to_3d to __all__ (module file appears missing). |
| src/physiomotion4d/workflow_convert_heart_gated_ct_to_usd.py | Uses new converter; multi-file path now uses itk.imread directly. |
| pyproject.toml | New physiomotion4d-convert-image-4d-to-3d script entry point (target module appears missing). |
| tests/test_convert_image_4d_to_3d.py | Renamed test suite using new class/methods. |
| tests/conftest.py | Fixture updated to new converter API. |
| tests/test_register_images_ants.py, test_register_images_icon.py, test_segment_chest_total_segmentator.py | Docstring updates referencing renamed test. |
| tests/README.md, TESTING_GUIDE.md, GITHUB_WORKFLOWS.md | Doc references updated to new test name. |
| docs/architecture.rst, testing.rst, API_MAP.md, cli_scripts/overview.rst, api/utilities/index.rst, api/utilities/image_conversion.rst | Docs renamed/updated for new module and CLI. |
| experiments/Heart-GatedCT_To_USD/0-download_and_convert_4d_to_3d.py, experiments/Heart-VTKSeries_To_USD/0-download_and_convert_4d_to_3d.py, data/Slicer-Heart-CT/download_and_convert.py | Example scripts updated to new API. |
| .github/workflows/ci.yml | CI runs renamed test file. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # CLI commands installed via pip | ||
| # Entry points reference the main() functions in the cli submodule | ||
| physiomotion4d-convert-ct-to-vtk = "physiomotion4d.cli.convert_ct_to_vtk:main" | ||
| physiomotion4d-convert-image-4d-to-3d = "physiomotion4d.cli.convert_image_4d_to_3d:main" |
|
|
||
| __all__ = [ | ||
| "convert_heart_gated_ct_to_usd", | ||
| "convert_image_4d_to_3d", |
Comment on lines
+38
to
+53
| def load_image_4d(self, filename: str) -> None: | ||
| """Load a 4D image and split it into a list of 3D ITK images. | ||
|
|
||
| ``.nrrd`` files (including Slicer ``.seq.nrrd`` heart sequences) are | ||
| read with ``pynrrd`` because their per-voxel vector dimension exceeds | ||
| the component counts wrapped in ITK Python. Every other format is | ||
| read with ``itk.imread`` and must already be a 4-dimensional image. | ||
|
|
||
| The result is a (T, Z, Y, X) ndarray plus 3D origin / spacing / | ||
| direction; each temporal slice becomes a standalone 3D ITK image in | ||
| ``self.img_3d``. | ||
|
|
||
| Args: | ||
| filename: Path to a 4D image file. | ||
| """ | ||
| if filename.lower().endswith(".nrrd"): |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/test_convert_image_4d_to_3d.py (1)
43-60:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMake
test_slice_files_createdself-contained to avoid order-dependent failures.Line 52 only inspects existing files and never performs conversion, so this test can fail when run alone (or before
test_convert_4d_to_3d). Generate slices in this test (or a fixture) before asserting counts.Proposed fix
def test_slice_files_created( self, download_test_data: Path, test_directories: dict[str, Path], ) -> None: """Test that all expected slice files are present after conversion.""" output_dir = test_directories["output"] / "convert_image_4d_to_3d" output_dir.mkdir(parents=True, exist_ok=True) + conv = ConvertImage4DTo3D() + conv.load_image_4d(str(download_test_data)) + conv.save_3d_images(output_dir, "slice") + expected_count = conv.get_number_of_3d_images() + slice_files = list(output_dir.glob("slice_*.mha")) - assert len(slice_files) > 10, ( - f"Expected more than 10 slice files, found {len(slice_files)}" - ) + assert len(slice_files) == expected_count, ( + f"Expected {expected_count} slice files, found {len(slice_files)}" + ) slice_007 = output_dir / "slice_007.mha" assert slice_007.exists(), "Expected slice_007.mha not found"🤖 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/test_convert_image_4d_to_3d.py` around lines 43 - 60, The test_slice_files_created test inspects output_dir but never runs the conversion, causing order-dependent failures; modify the test to first invoke the conversion step (call the conversion function or CLI entrypoint used elsewhere, e.g., the same routine exercised by test_convert_4d_to_3d) using download_test_data as input and output_dir as target so slices are generated in this test, then assert on list(output_dir.glob("slice_*.mha")) and slice_007.mha existence; keep the same output_dir creation (output_dir.mkdir(...)) and assertions but add the conversion call at the start of test_slice_files_created to make it self-contained.
🧹 Nitpick comments (3)
src/physiomotion4d/convert_image_4d_to_3d.py (3)
29-29: ⚡ Quick winUse
Union[int, str]for consistency.The coding guidelines specify
Optional[X]instead ofX | Noneto maintain compatibility. For consistency, union types should useUnion[int, str]rather thanint | str. As per coding guidelines, "UseOptional[X]instead ofX | None(ruffUP007suppressed)".♻️ Proposed fix
- def __init__(self, log_level: int | str = logging.INFO) -> None: + def __init__(self, log_level: Union[int, str] = logging.INFO) -> None:🤖 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 `@src/physiomotion4d/convert_image_4d_to_3d.py` at line 29, The __init__ annotation should use typing.Union for union types to match project conventions: change the signature of the constructor method __init__ in convert_image_4d_to_3d.py from "log_level: int | str" to "log_level: Union[int, str]" and add the corresponding "from typing import Union" import (or include Union in an existing typing import) so the type hint is consistent with the codebase guidelines.
97-99: ⚡ Quick winUse specific return type instead of
Any.The method returns an ITK image from
self.img_3d[index], which storesitk.Imageobjects (line 91). The return type should beitk.Imageinstead ofAnyfor type safety.♻️ Proposed fix
- def get_3d_image(self, index: int) -> Any: + def get_3d_image(self, index: int) -> itk.Image: """Return the 3D ITK image at the given time index.""" return self.img_3d[index]🤖 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 `@src/physiomotion4d/convert_image_4d_to_3d.py` around lines 97 - 99, The get_3d_image method currently declares a return type of Any but always returns an ITK image from self.img_3d; change the signature of get_3d_image to return itk.Image (or the specific itk.Image[PixelType, 3] if you know the pixel type) instead of Any and ensure the module imports itk so the type is available; update any type hints/usages that rely on the old Any to the new itk.Image return type (reference: method get_3d_image and attribute self.img_3d).
36-36: ⚡ Quick winReplace
Anywithitk.Imagefor type safety.The list stores ITK images (line 91 appends
itk.image_from_array(...)results), so the type should belist[itk.Image]instead oflist[Any].♻️ Proposed fix
- self.img_3d: list[Any] = [] + self.img_3d: list[itk.Image] = []🤖 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 `@src/physiomotion4d/convert_image_4d_to_3d.py` at line 36, Replace the broad annotation list[Any] on the instance variable self.img_3d with the specific itk image type by changing it to list[itk.Image]; ensure the module imports or references itk so the type is resolvable (e.g., import itk at top) and remove or adjust any unused Any imports—this narrows the type to the actual objects appended in convert_image_4d_to_3d (where itk.image_from_array(...) results are stored).
🤖 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 `@docs/api/utilities/image_conversion.rst`:
- Around line 7-8: The overview sentence currently states that all formats use
ITK readers; update it to clarify that while most 4D medical image formats
(NRRD, NIfTI, MHA, …) are read using ITK readers, NRRD may fall back to the
pynrrd reader for vector-valued data. Edit the top-line text in
image_conversion.rst (the Utilities for converting ... sentence) to explicitly
mention the NRRD/pynrrd exception so users aren’t misled.
In `@docs/cli_scripts/overview.rst`:
- Line 57: Update the sentence "Split a 4D medical image into a 3D time series
using ITK readers" to mention the NRRD fallback for vector-valued cases, e.g.,
rephrase to say it uses ITK readers where possible and falls back to NRRD
parsing for vector-valued images; ensure the new wording clearly communicates
both the primary ITK path and the NRRD exception for vector-valued data so
readers understand the fallback behavior.
In `@src/physiomotion4d/convert_image_4d_to_3d.py`:
- Around line 61-74: The code assumes required NRRD header fields exist when
building spacing_3d and direction_3d; add explicit validation around header
before using header["space origin"], header["space directions"], and
header["measurement frame"] in the function that computes
spacing_3d/direction_3d (the block that assigns spacing_3d, direction_3d and
inspects space). Check that header contains keys "space origin", "space
directions" (and that it has at least 4 rows/columns as used by header["space
directions"][x+1][x]) and "measurement frame" (and that its shape matches 3
elements); if any are missing or malformed raise a clear ValueError (or catch
and re-raise with a descriptive message) indicating the file is not a valid
Slicer .seq.nrrd, so callers of load_image_4d() get a user-friendly error
instead of a KeyError.
---
Outside diff comments:
In `@tests/test_convert_image_4d_to_3d.py`:
- Around line 43-60: The test_slice_files_created test inspects output_dir but
never runs the conversion, causing order-dependent failures; modify the test to
first invoke the conversion step (call the conversion function or CLI entrypoint
used elsewhere, e.g., the same routine exercised by test_convert_4d_to_3d) using
download_test_data as input and output_dir as target so slices are generated in
this test, then assert on list(output_dir.glob("slice_*.mha")) and slice_007.mha
existence; keep the same output_dir creation (output_dir.mkdir(...)) and
assertions but add the conversion call at the start of test_slice_files_created
to make it self-contained.
---
Nitpick comments:
In `@src/physiomotion4d/convert_image_4d_to_3d.py`:
- Line 29: The __init__ annotation should use typing.Union for union types to
match project conventions: change the signature of the constructor method
__init__ in convert_image_4d_to_3d.py from "log_level: int | str" to "log_level:
Union[int, str]" and add the corresponding "from typing import Union" import (or
include Union in an existing typing import) so the type hint is consistent with
the codebase guidelines.
- Around line 97-99: The get_3d_image method currently declares a return type of
Any but always returns an ITK image from self.img_3d; change the signature of
get_3d_image to return itk.Image (or the specific itk.Image[PixelType, 3] if you
know the pixel type) instead of Any and ensure the module imports itk so the
type is available; update any type hints/usages that rely on the old Any to the
new itk.Image return type (reference: method get_3d_image and attribute
self.img_3d).
- Line 36: Replace the broad annotation list[Any] on the instance variable
self.img_3d with the specific itk image type by changing it to list[itk.Image];
ensure the module imports or references itk so the type is resolvable (e.g.,
import itk at top) and remove or adjust any unused Any imports—this narrows the
type to the actual objects appended in convert_image_4d_to_3d (where
itk.image_from_array(...) results are stored).
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 352186cf-03a7-40e9-99c0-fce8b67b2984
📒 Files selected for processing (24)
.github/workflows/ci.ymldata/Slicer-Heart-CT/download_and_convert.pydocs/API_MAP.mddocs/api/utilities/image_conversion.rstdocs/api/utilities/index.rstdocs/architecture.rstdocs/cli_scripts/overview.rstdocs/testing.rstexperiments/Heart-GatedCT_To_USD/0-download_and_convert_4d_to_3d.pyexperiments/Heart-VTKSeries_To_USD/0-download_and_convert_4d_to_3d.pypyproject.tomlsrc/physiomotion4d/__init__.pysrc/physiomotion4d/cli/__init__.pysrc/physiomotion4d/convert_image_4d_to_3d.pysrc/physiomotion4d/convert_nrrd_4d_to_3d.pysrc/physiomotion4d/workflow_convert_heart_gated_ct_to_usd.pytests/GITHUB_WORKFLOWS.mdtests/README.mdtests/TESTING_GUIDE.mdtests/conftest.pytests/test_convert_image_4d_to_3d.pytests/test_register_images_ants.pytests/test_register_images_icon.pytests/test_segment_chest_total_segmentator.py
💤 Files with no reviewable changes (1)
- src/physiomotion4d/convert_nrrd_4d_to_3d.py
Comment on lines
+7
to
+8
| Utilities for converting 4D medical images (NRRD, NIfTI, MHA, …) into a 3D | ||
| time-series sequence using ITK readers. |
There was a problem hiding this comment.
Clarify NRRD handling exception in the overview text.
This currently implies all formats use ITK readers, but NRRD has a fallback path (pynrrd) for vector-valued data. Please update the sentence to avoid misleading users.
Suggested wording
-Utilities for converting 4D medical images (NRRD, NIfTI, MHA, …) into a 3D
-time-series sequence using ITK readers.
+Utilities for converting 4D medical images (NRRD, NIfTI, MHA, …) into a 3D
+time-series sequence, using ITK readers by default with NRRD-specific fallback handling where needed.📝 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
| Utilities for converting 4D medical images (NRRD, NIfTI, MHA, …) into a 3D | |
| time-series sequence using ITK readers. | |
| Utilities for converting 4D medical images (NRRD, NIfTI, MHA, …) into a 3D | |
| time-series sequence, using ITK readers by default with NRRD-specific fallback handling where needed. |
🤖 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 `@docs/api/utilities/image_conversion.rst` around lines 7 - 8, The overview
sentence currently states that all formats use ITK readers; update it to clarify
that while most 4D medical image formats (NRRD, NIfTI, MHA, …) are read using
ITK readers, NRRD may fall back to the pynrrd reader for vector-valued data.
Edit the top-line text in image_conversion.rst (the Utilities for converting ...
sentence) to explicitly mention the NRRD/pynrrd exception so users aren’t
misled.
| * - ``physiomotion4d-convert-ct-to-vtk`` | ||
| - Segment one CT image and export anatomy-group VTK surfaces and meshes | ||
| * - ``physiomotion4d-convert-image-4d-to-3d`` | ||
| - Split a 4D medical image into a 3D time series using ITK readers |
There was a problem hiding this comment.
Broaden description to include the NRRD fallback path.
The description currently states ITK readers only; this misses the NRRD exception path for vector-valued cases.
Suggested wording
- - Split a 4D medical image into a 3D time series using ITK readers
+ - Split a 4D medical image into a 3D time series (ITK by default, with NRRD-specific fallback handling)📝 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
| - Split a 4D medical image into a 3D time series using ITK readers | |
| - Split a 4D medical image into a 3D time series (ITK by default, with NRRD-specific fallback handling) |
🤖 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 `@docs/cli_scripts/overview.rst` at line 57, Update the sentence "Split a 4D
medical image into a 3D time series using ITK readers" to mention the NRRD
fallback for vector-valued cases, e.g., rephrase to say it uses ITK readers
where possible and falls back to NRRD parsing for vector-valued images; ensure
the new wording clearly communicates both the primary ITK path and the NRRD
exception for vector-valued data so readers understand the fallback behavior.
Comment on lines
+61
to
+74
| spacing_3d = np.array( | ||
| [abs(header["space directions"][x + 1][x]) for x in range(3)], | ||
| dtype=float, | ||
| ) | ||
| direction_3d = np.array( | ||
| [header["measurement frame"][x] for x in range(3)], dtype=float | ||
| ) | ||
| space = header.get("space", "") | ||
| if "right" in space: | ||
| direction_3d[0][0] *= -1 | ||
| if "anterior" in space: | ||
| direction_3d[1][1] *= -1 | ||
| if "inferior" in space: | ||
| direction_3d[2][2] *= -1 |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify NRRD header field structure and availability
# Find NRRD test files
fd -e nrrd -e seq.nrrd . --type f --max-results 3
# Check if pynrrd header access patterns are valid
python3 << 'PYTHON_EOF'
import nrrd
import sys
from pathlib import Path
# Find a test NRRD file
test_files = list(Path('.').rglob('*.nrrd'))
if not test_files:
print("No NRRD files found for verification")
sys.exit(0)
for test_file in test_files[:2]:
print(f"\n=== Checking {test_file} ===")
try:
data, header = nrrd.read(str(test_file))
print(f"Data shape: {data.shape}")
print(f"Data ndim: {data.ndim}")
# Check space directions
if "space directions" in header:
sd = header["space directions"]
print(f"space directions shape: {len(sd)} x {len(sd[0]) if sd else 0}")
print(f"space directions: {sd}")
else:
print("WARNING: 'space directions' not in header")
# Check measurement frame
if "measurement frame" in header:
mf = header["measurement frame"]
print(f"measurement frame: {mf}")
else:
print("WARNING: 'measurement frame' not in header")
# Check space origin
if "space origin" in header:
print(f"space origin: {header['space origin']}")
else:
print("WARNING: 'space origin' not in header")
# Check space field
if "space" in header:
print(f"space: {header['space']}")
else:
print("WARNING: 'space' not in header")
except Exception as e:
print(f"Error reading {test_file}: {e}")
PYTHON_EOFRepository: Project-MONAI/physiomotion4d
Length of output: 189
🏁 Script executed:
# Find and examine the file
fd convert_image_4d_to_3d.py --type fRepository: Project-MONAI/physiomotion4d
Length of output: 154
🏁 Script executed:
# Read the file to see the full function context
cat -n src/physiomotion4d/convert_image_4d_to_3d.py | head -100Repository: Project-MONAI/physiomotion4d
Length of output: 4715
🏁 Script executed:
# Search for test files related to this conversion
fd -e py -i test | xargs grep -l "convert_image_4d_to_3d\|load_image_4d" 2>/dev/null || echo "No direct test matches"Repository: Project-MONAI/physiomotion4d
Length of output: 243
🏁 Script executed:
# Check for NRRD test data files
find . -type f \( -name "*.nrrd" -o -name "*.seg.nrrd" \) 2>/dev/null | head -5Repository: Project-MONAI/physiomotion4d
Length of output: 54
🏁 Script executed:
# Search for other NRRD header access patterns in the codebase
rg "space directions|measurement frame" -A 2 -B 2Repository: Project-MONAI/physiomotion4d
Length of output: 881
🏁 Script executed:
cat -n tests/test_convert_image_4d_to_3d.pyRepository: Project-MONAI/physiomotion4d
Length of output: 4369
🏁 Script executed:
# Search for any NRRD test data or fixtures
fd -e nrrd --type f 2>/dev/null || echo "No NRRD files found"Repository: Project-MONAI/physiomotion4d
Length of output: 54
🏁 Script executed:
# Check conftest.py for test data setup
cat tests/conftest.pyRepository: Project-MONAI/physiomotion4d
Length of output: 19937
🏁 Script executed:
# Search for documentation or comments about NRRD header requirements
rg "space origin|space directions|measurement frame" -B 3 -A 3Repository: Project-MONAI/physiomotion4d
Length of output: 1254
🏁 Script executed:
# Search for error handling in NRRD-related code
rg "KeyError|get\(|try|except" -A 2 -B 2 src/physiomotion4d/convert_image_4d_to_3d.pyRepository: Project-MONAI/physiomotion4d
Length of output: 284
🏁 Script executed:
# Check pynrrd documentation or usage patterns in the project
rg "nrrd\." src/ -A 1 -B 1Repository: Project-MONAI/physiomotion4d
Length of output: 302
🏁 Script executed:
# Look for any NRRD validation or schema documentation
find . -name "*.md" -o -name "*.txt" | xargs grep -l "NRRD\|space directions\|measurement frame" 2>/dev/null || echo "No doc matches"Repository: Project-MONAI/physiomotion4d
Length of output: 132
🏁 Script executed:
# Check the Slicer-Heart-CT data download to understand expected format
rg "SLICER_HEART_CT\|DataDownloadTools" -A 5 -B 2Repository: Project-MONAI/physiomotion4d
Length of output: 54
🏁 Script executed:
# Check data README for NRRD format details
cat ./data/README.mdRepository: Project-MONAI/physiomotion4d
Length of output: 12123
🏁 Script executed:
# Check testing guide for NRRD requirements
cat ./tests/TESTING_GUIDE.mdRepository: Project-MONAI/physiomotion4d
Length of output: 11388
🏁 Script executed:
# Search for any NRRD format specifications in the codebase
rg "4D NRRD\|Slicer.*seq\|space directions" -A 3 -B 3Repository: Project-MONAI/physiomotion4d
Length of output: 54
🏁 Script executed:
# Look for any try-except blocks around nrrd header access to see if error handling exists
ast-grep --pattern 'try {
$$$
header["$_"]
$$$
} catch {
$$$
}'Repository: Project-MONAI/physiomotion4d
Length of output: 54
🏁 Script executed:
# Check if there are any comments explaining the NRRD format requirements
grep -n "pynrrd\|NRRD\|space directions\|measurement frame" src/physiomotion4d/convert_image_4d_to_3d.pyRepository: Project-MONAI/physiomotion4d
Length of output: 421
🏁 Script executed:
# Check if there are try-except blocks anywhere in the NRRD reading section
sed -n '53,96p' src/physiomotion4d/convert_image_4d_to_3d.py | grep -n "try\|except\|raise"Repository: Project-MONAI/physiomotion4d
Length of output: 109
🏁 Script executed:
# Look for any validation of NRRD headers elsewhere in the codebase
rg "header\[" src/ -B 1 -A 1 | head -30Repository: Project-MONAI/physiomotion4d
Length of output: 782
🏁 Script executed:
# Check if the function has ever been called with non-Slicer NRRD files
rg "load_image_4d" src/ tests/ -B 2 -A 2Repository: Project-MONAI/physiomotion4d
Length of output: 2717
🏁 Script executed:
# Check the complete function to see if there's any error handling around line 54 (nrrd.read)
sed -n '38,75p' src/physiomotion4d/convert_image_4d_to_3d.pyRepository: Project-MONAI/physiomotion4d
Length of output: 1725
🏁 Script executed:
# Verify that tests only use Slicer NRRD and not other NRRD types
rg "\.nrrd|\.seq\.nrrd" tests/ data/Repository: Project-MONAI/physiomotion4d
Length of output: 966
Add error handling for missing NRRD header fields.
The NRRD metadata extraction uses direct dictionary access (lines 60, 62, 66) without validation. While this function is specialized for Slicer .seq.nrrd files, it accepts any .nrrd file path and will raise a KeyError if required fields are missing:
header["space origin"](line 60)header["space directions"][x + 1][x](line 62)header["measurement frame"][x](line 66)
Since load_image_4d() is called from user-facing workflow code (workflow_convert_heart_gated_ct_to_usd.py), add try-except blocks or field existence checks before accessing these keys to provide clear error messages for invalid NRRD files.
🤖 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 `@src/physiomotion4d/convert_image_4d_to_3d.py` around lines 61 - 74, The code
assumes required NRRD header fields exist when building spacing_3d and
direction_3d; add explicit validation around header before using header["space
origin"], header["space directions"], and header["measurement frame"] in the
function that computes spacing_3d/direction_3d (the block that assigns
spacing_3d, direction_3d and inspects space). Check that header contains keys
"space origin", "space directions" (and that it has at least 4 rows/columns as
used by header["space directions"][x+1][x]) and "measurement frame" (and that
its shape matches 3 elements); if any are missing or malformed raise a clear
ValueError (or catch and re-raise with a descriptive message) indicating the
file is not a valid Slicer .seq.nrrd, so callers of load_image_4d() get a
user-friendly error instead of a KeyError.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #51 +/- ##
==========================================
+ Coverage 29.64% 30.10% +0.46%
==========================================
Files 48 48
Lines 6611 6616 +5
==========================================
+ Hits 1960 1992 +32
+ Misses 4651 4624 -27
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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
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.
Use ITK reader by default, except for NRRD which stores 4D data as images of vectors which is not available in ITK's python wrapping.
Summary by CodeRabbit
New Features
Documentation
Tests