Improve CuPy type hinting in ImageReader #8705
Conversation
📝 WalkthroughWalkthroughIntroduced a runtime typing alias Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧹 Recent nitpick comments
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Cache: Disabled due to data retention organization setting Knowledge base: Disabled due to 📒 Files selected for processing (1)
🧰 Additional context used📓 Path-based instructions (1)**/*.py⚙️ CodeRabbit configuration file
Files:
🔇 Additional comments (11)
✏️ Tip: You can disable this entire section by setting 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: 1
🤖 Fix all issues with AI agents
In `@monai/data/image_reader.py`:
- Around line 60-67: Remove the duplicated optional_import("cupy") call: keep
the first call that sets cp, has_cp and defines Ndarray (Union[np.ndarray,
cp.ndarray] or np.ndarray) and delete the second occurrence before kvikio
import; ensure the variables cp and has_cp are only defined once and then call
optional_import("kvikio") to assign kvikio, has_kvikio so there are no shadowed
or repeated imports.
🧹 Nitpick comments (7)
monai/data/image_reader.py (7)
119-123: Docstring inconsistent with return type.Line 122-123 says "numpy array" but return type is now
Ndarray(which includes CuPy). Update to "the first is an ndarray (numpy or cupy)".
281-293: Docstring at line 284 still references "numpy array".Same pattern as base class - update to reflect
Ndarrayreturn type.
628-632: Docstring at line 631 references "numpy array".Update to match return type.
1101-1105: Docstring at line 1104 references "numpy array".Update to match return type.
1287-1291: Docstring at line 1290 references "numpy array".Update to match return type. Note:
NumpyReaderonly returns numpy arrays in practice sincenp.loadis used, but type hint provides API consistency.
1380-1384: Docstring at line 1383 references "numpy array".Update to match return type.
1501-1505: Docstring at line 1504 references "numpy array".Update to match return type.
📜 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)
monai/data/image_reader.py
🧰 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:
monai/data/image_reader.py
🧬 Code graph analysis (1)
monai/data/image_reader.py (2)
monai/utils/module.py (1)
optional_import(315-445)monai/data/wsi_reader.py (1)
get_data(322-434)
⏰ 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: quick-py3 (macOS-latest)
- GitHub Check: quick-py3 (windows-latest)
- GitHub Check: flake8-py3 (pytype)
- GitHub Check: quick-py3 (ubuntu-latest)
- GitHub Check: packaging
- GitHub Check: build-docs
- GitHub Check: min-dep-py3 (3.10)
- GitHub Check: min-dep-pytorch (2.7.1)
- GitHub Check: min-dep-os (macOS-latest)
- GitHub Check: min-dep-pytorch (2.6.0)
- GitHub Check: min-dep-py3 (3.12)
- GitHub Check: min-dep-py3 (3.11)
- GitHub Check: min-dep-pytorch (2.5.1)
- GitHub Check: min-dep-pytorch (2.8.0)
- GitHub Check: min-dep-os (ubuntu-latest)
- GitHub Check: min-dep-py3 (3.9)
- GitHub Check: min-dep-os (windows-latest)
🔇 Additional comments (5)
monai/data/image_reader.py (5)
155-167: LGTM!Signature and logic correctly handle both numpy and cupy arrays.
849-858: LGTM!Type annotation and conditional allocation for CPU/GPU paths is correct.
908-961: LGTM!Return type correctly reflects the GPU loading path.
1192-1220: LGTM!GPU and CPU loading paths correctly return
Ndarray.
1427-1432: LGTM!Type update provides API consistency.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
ytl0623
force-pushed
the
fix-image_reader.py
branch
from
29ec762 to
5ca2eaa
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
monai/data/image_reader.py (5)
117-128: Docstring doesn't reflect the updated return type.Line 121 states "first is a numpy array" but the return type is now
Ndarraywhich includescupy.ndarray. Update the docstring to mention the array may be numpy or cupy.Suggested fix
def get_data(self, img) -> tuple[Ndarray, dict]: """ Extract data array and metadata from loaded image and return them. - This function must return two objects, the first is a numpy array of image data, + This function must return two objects, the first is a numpy or cupy array of image data, the second is a dictionary of metadata.
627-674: Docstring-type mismatch.Line 630 says "first is numpy array" but the return type and implementation support cupy arrays when
to_gpu=True.Suggested fix
def get_data(self, data) -> tuple[Ndarray, dict]: """ Extract data array and metadata from loaded image and return them. - This function returns two objects, first is numpy array of image data, second is dict of metadata. + This function returns two objects, first is numpy or cupy array of image data, second is dict of metadata.
1100-1112: Docstring-type mismatch.Line 1103 says "first is numpy array" but
Ndarraycovers cupy arrays whento_gpu=True.
1191-1219: Docstring says "Numpy array" but method can return cupy array.When
self.to_gpuis True, the method returns a cupy array, not numpy.
50-64: Consider adding type checking validation if cupy integration is critical.The
Ndarraytype alias is internal toimage_reader.py(not exported) and used only in method return type hints. While the reader classes are tested intests/data/test_image_rw.py, the conditional cupy import logic itself lacks explicit verification. If cupy availability detection is critical to the library's functionality, add a test verifying thatNdarraycorrectly resolves toUnion[np.ndarray, cp.ndarray]when cupy is available and tonp.ndarrayotherwise.
🧹 Nitpick comments (1)
monai/data/image_reader.py (1)
280-292: Docstring mentions numpy but type is Ndarray.Line 283 says "first is numpy array" while the return type is
Ndarray. Since ITKReader lacks GPU support, this is technically accurate at runtime, but for API consistency, consider updating to match other readers.
📜 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)
monai/data/image_reader.py
🧰 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:
monai/data/image_reader.py
🧬 Code graph analysis (1)
monai/data/image_reader.py (2)
monai/utils/module.py (1)
optional_import(315-445)monai/data/wsi_reader.py (1)
get_data(322-434)
🔇 Additional comments (8)
monai/data/image_reader.py (8)
40-66: TYPE_CHECKING pattern for Ndarray is sound.The dual definition approach correctly handles static analysis (using direct
cupyimport) while maintaining runtime compatibility viaoptional_import. The previous duplicateoptional_import("cupy")issue appears resolved.
154-166: LGTM!The
_stack_imagesfunction correctly handles both numpy and cupy arrays, with proper type annotations.
846-857: LGTM!The conditional GPU/CPU array allocation is correctly structured.
907-998: LGTM!Helper methods correctly typed with
Ndarrayreturn types.
1286-1298: Type annotation updated for consistency.NumpyReader lacks GPU support, so arrays remain numpy at runtime. The
Ndarraytype provides API consistency across readers.
1379-1393: LGTM!Type annotations updated for API consistency.
1426-1431: LGTM!NrrdImage.array typed as
Ndarrayfor consistency.
1500-1510: LGTM!Type annotations updated. NrrdReader is numpy-only at runtime.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
ytl0623
force-pushed
the
fix-image_reader.py
branch
from
5ca2eaa to
57a381b
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@monai/data/image_reader.py`:
- Around line 49-50: The chained assignment is invalid because the code uses a
comma before True, causing a tuple unpack into has_pydicom and has_cp; replace
the comma-based unpack with a proper chained assignment so all flags are set to
True (e.g., set has_nrrd = has_itk = has_nib = has_pil = has_pydicom = has_cp =
True) and leave the Ndarray type alias (Union[np.ndarray, cp.ndarray])
unchanged; update the TYPE_CHECKING block where variables has_nrrd, has_itk,
has_nib, has_pil, has_pydicom, has_cp and the Ndarray alias are defined.
🧹 Nitpick comments (1)
monai/data/image_reader.py (1)
118-123: Docstring still says “numpy array”.
Return type is nowNdarray(NumPy or CuPy). Update the base docstring and mirror in readers to avoid confusion.✏️ Suggested tweak
- This function must return two objects, the first is a numpy array of image data, + This function must return two objects, the first is a NumPy or CuPy array of image data,As per coding guidelines, update return docs to match behavior.
📜 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)
monai/data/image_reader.py
🧰 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:
monai/data/image_reader.py
🧬 Code graph analysis (1)
monai/data/image_reader.py (1)
monai/data/wsi_reader.py (1)
get_data(322-434)
⏰ 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 (pytype)
- GitHub Check: flake8-py3 (codeformat)
- GitHub Check: quick-py3 (windows-latest)
- GitHub Check: quick-py3 (ubuntu-latest)
- GitHub Check: build-docs
- GitHub Check: quick-py3 (macOS-latest)
- GitHub Check: flake8-py3 (mypy)
- GitHub Check: packaging
- GitHub Check: min-dep-py3 (3.12)
- GitHub Check: min-dep-os (windows-latest)
- GitHub Check: min-dep-pytorch (2.7.1)
- GitHub Check: min-dep-py3 (3.10)
- GitHub Check: min-dep-os (macOS-latest)
- GitHub Check: min-dep-py3 (3.9)
- GitHub Check: min-dep-os (ubuntu-latest)
- GitHub Check: min-dep-pytorch (2.8.0)
- GitHub Check: min-dep-py3 (3.11)
- GitHub Check: min-dep-pytorch (2.6.0)
- GitHub Check: min-dep-pytorch (2.5.1)
🔇 Additional comments (4)
monai/data/image_reader.py (4)
25-26: Conditional typing plumbing looks fine.Also applies to: 40-45, 60-65
154-166: LGTM.
280-292: Ndarray propagation inget_dataflows looks consistent.Also applies to: 627-674, 1100-1113, 1286-1299, 1379-1394, 1500-1511
849-852: Helper/struct annotations look consistent.Also applies to: 907-998, 1191-1219, 1430-1431
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
ytl0623
force-pushed
the
fix-image_reader.py
branch
from
57a381b to
a1e0196
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
monai/data/image_reader.py (1)
154-166: Add a docstring for_stack_images.
Helper is now type-annotated; add Google‑style Args/Returns. As per coding guidelines.Suggested docstring
def _stack_images(image_list: list[Ndarray], meta_dict: dict, to_cupy: bool = False) -> Ndarray: + """ + Stack image arrays and update channel metadata. + + Args: + image_list: List of image arrays to stack. + meta_dict: Metadata dict to update. + to_cupy: If True, stack using CuPy. + + Returns: + The stacked image array. + """
🤖 Fix all issues with AI agents
In `@monai/data/image_reader.py`:
- Around line 118-123: The get_data method's docstring currently describes the
return types in prose but lacks a Google‑style "Returns:" section; update
get_data to include an explicit Returns section that states the return type
tuple[Ndarray, dict] and documents each element (first: NumPy/CuPy array of
image data, second: dict of metadata) in Google docstring format, and apply the
same explicit Returns block to the other updated image-reading methods in this
file so all modified methods consistently include a Returns section.
📜 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)
monai/data/image_reader.py
🧰 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:
monai/data/image_reader.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: quick-py3 (ubuntu-latest)
- GitHub Check: quick-py3 (macOS-latest)
- GitHub Check: flake8-py3 (mypy)
- GitHub Check: packaging
- GitHub Check: flake8-py3 (pytype)
- GitHub Check: flake8-py3 (codeformat)
- GitHub Check: quick-py3 (windows-latest)
- GitHub Check: build-docs
- 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-os (macOS-latest)
- GitHub Check: min-dep-pytorch (2.7.1)
- GitHub Check: min-dep-pytorch (2.8.0)
- GitHub Check: min-dep-pytorch (2.6.0)
- GitHub Check: min-dep-os (windows-latest)
- GitHub Check: min-dep-py3 (3.11)
- GitHub Check: min-dep-py3 (3.9)
- GitHub Check: min-dep-os (ubuntu-latest)
🔇 Additional comments (2)
monai/data/image_reader.py (2)
25-64: Ndarray alias + optional CuPy import look solid.
TYPE_CHECKING + optional_import keeps runtime safe while enabling CuPy typing; please sanity-check current CuPy typing stubs thatcupy.ndarrayremains the public array type.
292-292: Ndarray propagation across readers is consistent.
Updated list/field/return hints read cleanly; please verify type-checking with and without CuPy installed.Also applies to: 674-674, 849-850, 907-907, 962-962, 1112-1112, 1191-1191, 1298-1298, 1393-1393, 1430-1430, 1510-1510
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
Signed-off-by: ytl0623 <david89062388@gmail.com>
ytl0623
force-pushed
the
fix-image_reader.py
branch
from
a1e0196 to
8a07c9e
Compare
1 participant
Related to #8188
Description
This PR addresses the comment in
monai/data/image_reader.pyregarding the correct type annotation for image arrays that can be eithernp.ndarrayorcp.ndarray.Previously, the type hint was restricted to
np.ndarrayto avoid ImportError for users without CuPy installed. This PR introduces a robust Ndarray type alias that supports static type checking forcupy.ndarraywhile maintaining runtime compatibility for CPU-only environments.Types of changes
./runtests.sh -f -u --net --coverage../runtests.sh --quick --unittests --disttests.make htmlcommand in thedocs/folder.