-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Fix type hint for image reader #8646
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: dev
Are you sure you want to change the base?
Conversation
WalkthroughThe change adds a Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 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)
🚧 Files skipped from review as they are similar to previous changes (1)
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
monai/data/image_reader.py (1)
25-25: NdarrayOrCupy alias looks correct; only minor typing/style nits.The
TYPE_CHECKING/runtime-Anypattern forNdarrayOrCupycleanly supports optional CuPy and avoids runtime import issues. This should fix the local type errors around CuPy arrays without changing behavior. As a small follow‑up you could (optionally) switch to the|union syntax (NdarrayOrCupy = np.ndarray | cupy.ndarray) and then drop theUnionimport for consistency with the rest of the file, and consider whether you eventually want publicget_datareturn annotations/docstrings to reflect that GPU paths may yield CuPy arrays rather than strictly NumPy.Also applies to: 60-65
📜 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(4 hunks)
🧰 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: build-docs
- 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-pytorch (2.5.1)
- GitHub Check: min-dep-py3 (3.11)
- GitHub Check: min-dep-py3 (3.12)
- GitHub Check: min-dep-os (ubuntu-latest)
- GitHub Check: flake8-py3 (pytype)
- GitHub Check: flake8-py3 (mypy)
- GitHub Check: quick-py3 (macOS-latest)
- GitHub Check: packaging
- GitHub Check: min-dep-os (windows-latest)
- GitHub Check: quick-py3 (ubuntu-latest)
- GitHub Check: min-dep-os (macOS-latest)
- GitHub Check: min-dep-py3 (3.10)
- GitHub Check: flake8-py3 (codeformat)
- GitHub Check: quick-py3 (windows-latest)
- GitHub Check: min-dep-py3 (3.9)
🔇 Additional comments (2)
monai/data/image_reader.py (2)
673-701: PydicomReader local buffer typing now matches possible NumPy/CuPy values.Updating
img_arraytolist[NdarrayOrCupy]matches the actual values appended in both CPU and GPU (to_gpu) paths and removes the previous type mismatch while leaving runtime unchanged.
1111-1138: NibabelReader local buffer typing correctly generalized to NumPy/CuPy.Using
list[NdarrayOrCupy]forimg_arrayhere is consistent with_get_array_dataand_stack_imageswhento_gpu=Trueand aligns the internal typing with existing GPU behavior.
Signed-off-by: Tristan Kirscher <[email protected]>
e242c44 to
bf06a4f
Compare
ericspod
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @Kirscher thanks for these updates, I think it's fine though I agree we need a better way of doing the type checking when optional libraries are used.
| if TYPE_CHECKING: | ||
| import cupy | ||
|
|
||
| NdarrayOrCupy = Union[np.ndarray, cupy.ndarray] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was done to get around Cupy being an optional dependency, this makes it necessary when type checking but this might not be avoidable right now. We do need to consider a way of defining placeholder types for optional dependencies so we can type check correctly without these workarounds.
Description
This PR fixes the type hint for image reader to correctly handle Cupy arrays and removes the TODO.
Types of changes