Fix: make y optional in ignore_background#8647
Fix: make y optional in ignore_background#8647Kirscher wants to merge 3 commits intoProject-MONAI:devfrom
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration 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)
📝 WalkthroughWalkthroughThe Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
Signed-off-by: Tristan Kirscher <85068746+Kirscher@users.noreply.github.com>
Kirscher
force-pushed
the
fix/ignore-background-optional-y
branch
from
2a9b490 to
74ce60d
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: 0
🧹 Nitpick comments (3)
monai/metrics/utils.py (1)
54-56: Align ignore_background docstring with new optionalybehaviorThe new signature/guard for optional
ylooks correct and keeps existing callers safe. The docstring still assumesyis always present and doesn’t mention that the second tuple element can beNone, and there’s no Returns section.Consider tightening the docs, e.g.:
Args: y: optional ground truth; if provided, shape matches `y_pred` and the first dim is batch. If None, only `y_pred` is modified. Returns: Tuple of (`y_pred` without background, `y` without background or None).Also, ensure you have at least one test hitting
ignore_background(..., y=None)(e.g., via thecompute_variance(include_background=False, ...)path).Also applies to: 68-71
monai/data/image_reader.py (2)
25-26: NdarrayOrCupy alias is reasonable; minor style thoughtThe TYPE_CHECKING-only
NdarrayOrCupy = Union[np.ndarray, cupy.ndarray]with a runtimeAnyfallback is a sensible way to avoid a hard CuPy dependency while keeping type checkers happy.If you touch this again, you might consider PEP 604 syntax for consistency with the rest of the file, e.g.
NdarrayOrCupy = np.ndarray | cupy.ndarray, but it’s purely cosmetic.Also applies to: 60-66
673-701: Broaden get_data typing/docs to match possible CuPy returnsSwitching
img_arraytolist[NdarrayOrCupy]inPydicomReader.get_dataandNibabelReader.get_datacorrectly reflects that elements may be NumPy or CuPy arrays whento_gpu=True. However:
- Both methods are still annotated as returning
tuple[np.ndarray, dict].- Their docstrings also describe the first return as “numpy array of image data”.
Given that these paths can now yield CuPy arrays, consider (in a follow-up) widening the public typing/docs, for example:
- Updating the return types to
tuple[NdarrayOrCupy, dict](and adjustingImageReader.get_dataif you want consistent overrides), and- Relaxing the docstrings to “array-like image data (NumPy or CuPy)”.
This will help static checkers and users relying on the type hints, without changing runtime behavior.
Also applies to: 1111-1138
📜 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 (3)
monai/data/image_reader.py(4 hunks)monai/metrics/active_learning_metrics.py(1 hunks)monai/metrics/utils.py(2 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.pymonai/metrics/utils.pymonai/metrics/active_learning_metrics.py
🧬 Code graph analysis (1)
monai/metrics/active_learning_metrics.py (1)
monai/metrics/utils.py (1)
ignore_background(54-71)
⏰ 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: min-dep-pytorch (2.8.0)
- GitHub Check: min-dep-pytorch (2.7.1)
- GitHub Check: min-dep-py3 (3.12)
- GitHub Check: min-dep-pytorch (2.5.1)
- GitHub Check: min-dep-py3 (3.9)
- GitHub Check: min-dep-pytorch (2.6.0)
- GitHub Check: min-dep-py3 (3.10)
- GitHub Check: build-docs
- GitHub Check: packaging
- GitHub Check: flake8-py3 (mypy)
- GitHub Check: min-dep-py3 (3.11)
- GitHub Check: min-dep-os (ubuntu-latest)
- GitHub Check: flake8-py3 (pytype)
- GitHub Check: flake8-py3 (codeformat)
- GitHub Check: min-dep-os (macOS-latest)
- GitHub Check: quick-py3 (macOS-latest)
- GitHub Check: quick-py3 (ubuntu-latest)
- GitHub Check: min-dep-os (windows-latest)
- GitHub Check: quick-py3 (windows-latest)
🔇 Additional comments (1)
monai/metrics/active_learning_metrics.py (1)
128-136: ignore_background usage incompute_variancelooks correctPassing
y=Noneand discarding the second return cleanly uses the new optional parameter while preserving the existing “drop channel 0” behavior ony_pred.Please just confirm there’s coverage for the
include_background=Falsebranch (including multi-channel and single-channel cases) in the variance tests.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
monai/metrics/active_learning_metrics.py (1)
131-132: Background removal call is correct;y=Noneis redundantThe new call correctly matches the updated
ignore_backgroundsignature and behavior. You can simplify slightly by relying on the default fory:- if not include_background: - y_pred, _ = ignore_background(y_pred=y_pred, y=None) + if not include_background: + y_pred, _ = ignore_background(y_pred=y_pred)
📜 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 (2)
monai/metrics/active_learning_metrics.py(1 hunks)monai/metrics/utils.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- monai/metrics/utils.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/metrics/active_learning_metrics.py
🧬 Code graph analysis (1)
monai/metrics/active_learning_metrics.py (1)
monai/metrics/utils.py (1)
ignore_background(54-71)
⏰ 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: min-dep-py3 (3.10)
- GitHub Check: min-dep-pytorch (2.8.0)
- GitHub Check: min-dep-py3 (3.9)
- GitHub Check: min-dep-pytorch (2.7.1)
- GitHub Check: min-dep-py3 (3.11)
- GitHub Check: min-dep-py3 (3.12)
- GitHub Check: min-dep-pytorch (2.6.0)
- GitHub Check: min-dep-os (ubuntu-latest)
- GitHub Check: min-dep-pytorch (2.5.1)
- GitHub Check: min-dep-os (windows-latest)
- GitHub Check: min-dep-os (macOS-latest)
- GitHub Check: quick-py3 (macOS-latest)
- GitHub Check: packaging
- GitHub Check: build-docs
- GitHub Check: quick-py3 (ubuntu-latest)
- GitHub Check: quick-py3 (windows-latest)
- GitHub Check: flake8-py3 (codeformat)
- GitHub Check: flake8-py3 (mypy)
- GitHub Check: flake8-py3 (pytype)
There was a problem hiding this comment.
Pull request overview
Makes ignore_background more flexible by allowing callers to drop the y argument, and updates active learning variance computation to use the new optional-y behavior.
Changes:
- Updated
ignore_backgroundto accepty: ... | Noneand conditionally strip the background channel fromyonly when provided. - Simplified
compute_varianceto remove background only fromy_predwithout constructing a dummyy.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
monai/metrics/utils.py |
Updates ignore_background signature/behavior to support y=None. |
monai/metrics/active_learning_metrics.py |
Uses the new optional-y behavior when excluding background in compute_variance. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| def ignore_background( | ||
| y_pred: NdarrayTensor, y: NdarrayTensor | None = None | ||
| ) -> tuple[NdarrayTensor, NdarrayTensor | None]: |
There was a problem hiding this comment.
The updated type signature makes y optional, but it also changes the return type for all callers to tuple[NdarrayTensor, NdarrayTensor | None]. This will propagate Optional[...] into existing call sites that always pass y (many metrics do), and is likely to fail the repo’s mypy checks. Consider using @overload so that when y is provided the return type remains tuple[NdarrayTensor, NdarrayTensor], and only returns None in the second position when y is omitted/None.
There was a problem hiding this comment.
Hi @Kirscher we are seeing this issue in the mypy tests now, please look at what was said here to see how to resolve it. There are other uses of ignore_background you can update to remove the unneeded y=y argument.
| @@ -63,7 +65,8 @@ def ignore_background(y_pred: NdarrayTensor, y: NdarrayTensor) -> tuple[NdarrayT | |||
|
|
|||
There was a problem hiding this comment.
ignore_background now allows y=None, but the docstring still describes y as a required ground truth tensor. Please update the docstring to reflect that y is optional and clarify what is returned when y is None (e.g., second tuple element is None).
| y = y_pred | ||
| # TODO If this utils is made to be optional for 'y' it would be nice | ||
| y_pred, y = ignore_background(y_pred=y_pred, y=y) | ||
| y_pred, _ = ignore_background(y_pred=y_pred, y=None) |
There was a problem hiding this comment.
PR description indicates new tests were added for this change, but the diff only modifies monai/metrics/utils.py and monai/metrics/active_learning_metrics.py (no test updates shown). If there are no test changes, please update the PR description checkbox accordingly, or include the new/updated test(s) in this PR.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Signed-off-by: Eric Kerfoot <17726042+ericspod@users.noreply.github.com>
Fixes # .
Description
This PR addresses a TODO in
monai/metrics/active_learning_metrics.pyby making theyargument optional in theignore_backgroundutility function. This allows for cleaner code when onlyy_predneeds to be processed, as seen inactive_learning_metrics.py.Types of changes
./runtests.sh -f -u --net --coverage../runtests.sh --quick --unittests --disttests.make htmlcommand in thedocs/folder.