Update the docstrings to match the actual code defaults#8701
Update the docstrings to match the actual code defaults#8701ericspod merged 9 commits intoProject-MONAI:devfrom
Conversation
📝 WalkthroughWalkthroughUpdated the public API and docstrings in monai/losses/unified_focal_loss.py. The AsymmetricUnifiedFocalLoss constructor signature was changed to include new parameters Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 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)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. 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/losses/unified_focal_loss.py`:
- Around line 171-172: The docstring for the weight parameter is out of sync
with the function signature: update the docstring for the weight parameter in
UnifiedFocalLoss (the parameter named weight in the constructor/signature) to
reflect the actual default of 0.5 (e.g., "Defaults to 0.5.") so it matches the
signature where weight: float = 0.5.
📜 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/losses/unified_focal_loss.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/losses/unified_focal_loss.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: min-dep-py3 (3.12)
- GitHub Check: min-dep-pytorch (2.8.0)
- GitHub Check: min-dep-pytorch (2.7.1)
- 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-py3 (3.9)
- GitHub Check: min-dep-py3 (3.11)
- GitHub Check: min-dep-py3 (3.10)
- GitHub Check: min-dep-os (macOS-latest)
- GitHub Check: min-dep-os (windows-latest)
- GitHub Check: quick-py3 (macOS-latest)
- GitHub Check: flake8-py3 (mypy)
- GitHub Check: flake8-py3 (pytype)
- GitHub Check: packaging
- GitHub Check: quick-py3 (ubuntu-latest)
- GitHub Check: build-docs
- GitHub Check: quick-py3 (windows-latest)
- GitHub Check: flake8-py3 (codeformat)
🔇 Additional comments (1)
monai/losses/unified_focal_loss.py (1)
111-111: Docstring now matches code default.Correctly aligned with
gamma: float = 2on line 103.
✏️ 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-issue-8689
branch
from
6925d03 to
618074b
Compare
Signed-off-by: ytl0623 <david89062388@gmail.com>
|
Hi @ericspod, please take a look at the code when you're free. Once the CI/CD is green, we can go ahead and merge it. (Similar fixes in other PRs are being merged soon as well.) Thanks in advance! |
|
Hi @ytl0623 I will soon have a chance to come back to this, sorry for the delay but we have another CI issue with our Docker image we're sorting. |
Signed-off-by: Eric Kerfoot <17726042+ericspod@users.noreply.github.com>
Signed-off-by: ytl0623 <david89062388@gmail.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 (2)
monai/losses/unified_focal_loss.py (2)
166-183:⚠️ Potential issue | 🟡 Minor
reductionparameter missing from docstring; extra blank line.Two issues:
- Constructor has
reduction(line 164) but the docstring doesn't document it — coding guideline violation.- Two consecutive blank lines after
delta(lines 173–174) where theepsilonentry was removed; one should go.📝 Proposed fix
delta : weight of the background. Defaults to 0.7. - - + reduction : specifies the reduction to apply to the output. Defaults to ``"mean"``. Example:As per coding guidelines, docstrings should describe each variable in the appropriate section.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@monai/losses/unified_focal_loss.py` around lines 166 - 183, Update the AsymmetricUnifiedFocalLoss docstring to include the missing reduction parameter and remove the extra blank line: add a short entry under Args describing the reduction argument (accepted values like "none", "mean", "sum" or match the constructor behavior) and its default, and delete the stray empty line after the delta description so the Args block is contiguous (this touches the class/method docstring for AsymmetricUnifiedFocalLoss in unified_focal_loss.py and corresponds to the constructor parameters including reduction and the removed epsilon).
157-191:⚠️ Potential issue | 🟡 MinorNew
weightandgammaparameters need test coverage.
weightandgammaare now explicit, user-facing constructor parameters but no tests exercise non-default values (e.g.,weight=0.0/1.0, differentgamma).As per coding guidelines, new or modified definitions should be covered by existing or new unit tests.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@monai/losses/unified_focal_loss.py` around lines 157 - 191, Add unit tests exercising the new constructor parameters on AsymmetricUnifiedFocalLoss: create tests that instantiate AsymmetricUnifiedFocalLoss with varied weight values (e.g., 0.0 and 1.0) and with different gamma values (non-default) and assert expected behavioral differences (for example, loss outputs change accordingly for identical pred/target tensors or that weight=0.0 yields zero contribution from the weighted term); ensure tests call the class __init__ and forward (fl(pred, grnd)) and validate internal attributes (weight, gamma) and resulting loss values to cover both parameter parsing and numeric effect.
🤖 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/losses/unified_focal_loss.py`:
- Around line 166-183: Update the AsymmetricUnifiedFocalLoss docstring to
include the missing reduction parameter and remove the extra blank line: add a
short entry under Args describing the reduction argument (accepted values like
"none", "mean", "sum" or match the constructor behavior) and its default, and
delete the stray empty line after the delta description so the Args block is
contiguous (this touches the class/method docstring for
AsymmetricUnifiedFocalLoss in unified_focal_loss.py and corresponds to the
constructor parameters including reduction and the removed epsilon).
- Around line 157-191: Add unit tests exercising the new constructor parameters
on AsymmetricUnifiedFocalLoss: create tests that instantiate
AsymmetricUnifiedFocalLoss with varied weight values (e.g., 0.0 and 1.0) and
with different gamma values (non-default) and assert expected behavioral
differences (for example, loss outputs change accordingly for identical
pred/target tensors or that weight=0.0 yields zero contribution from the
weighted term); ensure tests call the class __init__ and forward (fl(pred,
grnd)) and validate internal attributes (weight, gamma) and resulting loss
values to cover both parameter parsing and numeric effect.
…I#8701) Fixes Project-MONAI#8689 ### Description Update the docstrings to match the actual code defaults. 1. AsymmetricFocalLoss (line 103, 111): `gamma` 2. AsymmetricUnifiedFocalLoss (line 162, 171): `gamma` 3. AsymmetricUnifiedFocalLoss (line 172): `epsilon` ### Types of changes <!--- Put an `x` in all the boxes that apply, and remove the not applicable items --> - [x] Non-breaking change (fix or new feature that would not break existing functionality). - [ ] Breaking change (fix or new feature that would cause existing functionality to change). - [ ] New tests added to cover the changes. - [ ] Integration tests passed locally by running `./runtests.sh -f -u --net --coverage`. - [ ] Quick tests passed locally by running `./runtests.sh --quick --unittests --disttests`. - [ ] In-line docstrings updated. - [ ] Documentation updated, tested `make html` command in the `docs/` folder. --------- Signed-off-by: ytl0623 <david89062388@gmail.com> Signed-off-by: Eric Kerfoot <17726042+ericspod@users.noreply.github.com> Co-authored-by: Eric Kerfoot <17726042+ericspod@users.noreply.github.com>
…I#8701) Fixes Project-MONAI#8689 ### Description Update the docstrings to match the actual code defaults. 1. AsymmetricFocalLoss (line 103, 111): `gamma` 2. AsymmetricUnifiedFocalLoss (line 162, 171): `gamma` 3. AsymmetricUnifiedFocalLoss (line 172): `epsilon` ### Types of changes <!--- Put an `x` in all the boxes that apply, and remove the not applicable items --> - [x] Non-breaking change (fix or new feature that would not break existing functionality). - [ ] Breaking change (fix or new feature that would cause existing functionality to change). - [ ] New tests added to cover the changes. - [ ] Integration tests passed locally by running `./runtests.sh -f -u --net --coverage`. - [ ] Quick tests passed locally by running `./runtests.sh --quick --unittests --disttests`. - [ ] In-line docstrings updated. - [ ] Documentation updated, tested `make html` command in the `docs/` folder. --------- Signed-off-by: ytl0623 <david89062388@gmail.com> Signed-off-by: Eric Kerfoot <17726042+ericspod@users.noreply.github.com> Co-authored-by: Eric Kerfoot <17726042+ericspod@users.noreply.github.com>
2 participants
Fixes #8689
Description
Update the docstrings to match the actual code defaults.
gammagammaepsilonTypes of changes
./runtests.sh -f -u --net --coverage../runtests.sh --quick --unittests --disttests.make htmlcommand in thedocs/folder.