Fix: make y optional in ignore_background#8647
Fix: make y optional in ignore_background#8647Kirscher wants to merge 4 commits intoProject-MONAI:devfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe 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🧪 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>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
monai/metrics/utils.py (1)
54-66:⚠️ Potential issue | 🟡 MinorDocstring needs update for optional
yand return values.Per coding guidelines, the docstring should document:
- That
yis optional (defaultNone)- Return values (what the tuple contains)
- Behavior when
yisNone📝 Proposed docstring update
def ignore_background( y_pred: NdarrayTensor, y: NdarrayTensor | None = None ) -> tuple[NdarrayTensor, NdarrayTensor | None]: """ This function is used to remove background (the first channel) for `y_pred` and `y`. Args: y_pred: predictions. As for classification tasks, `y_pred` should has the shape [BN] where N is larger than 1. As for segmentation tasks, the shape should be [BNHW] or [BNHWD]. - y: ground truth, the first dim is batch. + y: ground truth, the first dim is batch. If None, only `y_pred` is processed. + + Returns: + A tuple of (y_pred, y) with the background channel removed. If `y` is None, + returns (y_pred, None). """As per coding guidelines: "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."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@monai/metrics/utils.py` around lines 54 - 66, Update the docstring for function ignore_background to explicitly state that parameter y is optional and defaults to None, describe the return value as a tuple (y_pred_no_bg, y_no_bg_or_None) where the first element is y_pred with the background/first channel removed and the second is y with background removed or None if no y was provided, and document the function behavior when y is None (i.e., only y_pred is processed and the second tuple element is None); include these descriptions in the Args and Returns sections of the Google-style docstring for ignore_background.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@monai/metrics/utils.py`:
- Around line 54-66: Update the docstring for function ignore_background to
explicitly state that parameter y is optional and defaults to None, describe the
return value as a tuple (y_pred_no_bg, y_no_bg_or_None) where the first element
is y_pred with the background/first channel removed and the second is y with
background removed or None if no y was provided, and document the function
behavior when y is None (i.e., only y_pred is processed and the second tuple
element is None); include these descriptions in the Args and Returns sections of
the Google-style docstring for ignore_background.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d0815cec-5bab-444a-8bc3-f2da55312bf9
📒 Files selected for processing (1)
monai/metrics/utils.py
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.