Fix network_type docstring formatting in PerceptualLoss #8670
Conversation
📝 WalkthroughWalkthroughDocstring-only edits in monai/losses/perceptual.py: Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
📜 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 |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
monai/losses/perceptual.py (1)
75-75: Update type annotation to match accepted types.Type annotation specifies
strbut should bestr | PercetualNetworkTypeto match the docstring and actual usage.🔎 Proposed fix
- network_type: str = PercetualNetworkType.alex, + network_type: str | PercetualNetworkType = PercetualNetworkType.alex,
📜 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/perceptual.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/perceptual.py
🔇 Additional comments (2)
monai/losses/perceptual.py (2)
52-56: Docstring update is accurate but type annotation doesn't match.The docstring correctly documents that
network_typeaccepts bothstrandPercetualNetworkType, but the type annotation on line 75 only specifiesstr.
98-102: No issues found.StrEnummembers inherit fromstr, so.lower()works on enum inputs and comparison againstlist(PercetualNetworkType)succeeds for both string and enum inputs. Logic is correct as written.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 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/perceptual.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/perceptual.py
🧬 Code graph analysis (1)
monai/losses/perceptual.py (1)
monai/utils/enums.py (1)
StrEnum(68-90)
🔇 Additional comments (1)
monai/losses/perceptual.py (1)
52-60: LGTM: Improved docstring clarity.The reformatted list of valid
network_typevalues is clearer than the previous inline notation.
|
Hi @virginiafdez, The spelling/enum issue in PerceptualLoss has been fixed (PercetualNetworkType → PerceptualNetworkType), and all references/docstrings have been updated accordingly. Additionally, all missing DCO sign-offs have been added via remediation commits. Everything should now be in order for review. |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 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)
e --abort
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
monai/data/image_writer.py (1)
119-128: Move dictionary to module level.The dictionary is recreated on every call. Define it as a module-level constant.
🔎 Proposed refactor
Move the dictionary definition outside the function, near the top of the file after line 64:
SUPPORTED_WRITERS: dict = {} _RECOMMENDED_PACKAGES = { "png": "Pillow", "jpg": "Pillow", "jpeg": "Pillow", "nii": "nibabel or SimpleITK", "niigz": "nibabel or SimpleITK", "nrrd": "pynrrd", "tif": "Pillow or tifffile", "tiff": "Pillow or tifffile", }Then reference it in the function at line 131:
- RECOMMENDED_PACKAGES = { - "png": "Pillow", - "jpg": "Pillow", - "jpeg": "Pillow", - "nii": "nibabel or SimpleITK", - "nii.gz": "nibabel or SimpleITK", - "nrrd": "pynrrd", - "tif": "Pillow or tifffile", - "tiff": "Pillow or tifffile", - } - fmt_clean = fmt.replace(".", "").lower() - package_hint = RECOMMENDED_PACKAGES.get(fmt_clean, "") + package_hint = _RECOMMENDED_PACKAGES.get(fmt_clean, "")Note: Also see the critical bug in line 130 regarding "nii.gz" handling.
📜 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/data/image_writer.pymonai/losses/perceptual.py
🚧 Files skipped from review as they are similar to previous changes (1)
- monai/losses/perceptual.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_writer.py
🧬 Code graph analysis (1)
monai/data/image_writer.py (1)
monai/utils/module.py (1)
OptionalImportError(309-312)
⏰ 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). (18)
- GitHub Check: min-dep-py3 (3.12)
- GitHub Check: min-dep-py3 (3.10)
- GitHub Check: min-dep-pytorch (2.7.1)
- 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.11)
- GitHub Check: min-dep-os (windows-latest)
- GitHub Check: min-dep-pytorch (2.8.0)
- GitHub Check: min-dep-os (ubuntu-latest)
- GitHub Check: min-dep-os (macOS-latest)
- GitHub Check: quick-py3 (windows-latest)
- GitHub Check: quick-py3 (ubuntu-latest)
- GitHub Check: flake8-py3 (pytype)
- GitHub Check: flake8-py3 (codeformat)
- GitHub Check: quick-py3 (macOS-latest)
- GitHub Check: flake8-py3 (mypy)
- GitHub Check: packaging
aman0311x
force-pushed
the
dev
branch
3 times, most recently
from
7d763b1 to
3d3b0cc
Compare
|
The CI is consistently failing at the dependency installation step with: All other jobs have completed successfully. |
ericspod
left a comment
ericspod
left a comment
There was a problem hiding this comment.
Hi @aman0311x thanks for this contribution. I had a very minor comment but otherwise I think it's good to go. Please update when you can and hopefully we'll have the CICD issues sorted.
|
All CI/CD checks have passed successfully . Ready for merge! @ericspod |
|
Hi @aman0311x, could you please help resolve the confict, then I will trigger the blossom to merge the pr, thanks! |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @monai/losses/perceptual.py:
- Around line 52-60: Update the docstring for the network_type parameter to use
the corrected enum name PerceptualNetworkType (replace the misspelled
PercetualNetworkType) and convert the current type+list block into a
Google-style parameter description: remove the inline type signature and instead
document network_type: PerceptualNetworkType — one of "alex", "vgg", "squeeze",
"radimagenet_resnet50", "medicalnet_resnet10_23datasets",
"medicalnet_resnet50_23datasets", or "resnet50". Also ensure the default is
noted (PerceptualNetworkType.alex) and the docstring matches the function or
class that defines network_type (search for network_type and
PerceptualNetworkType in perceptual.py).
📜 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/perceptual.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/perceptual.py
🧬 Code graph analysis (1)
monai/losses/perceptual.py (1)
monai/utils/enums.py (1)
StrEnum(68-90)
⏰ 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: flake8-py3 (mypy)
- GitHub Check: flake8-py3 (codeformat)
- GitHub Check: flake8-py3 (pytype)
- GitHub Check: quick-py3 (windows-latest)
- GitHub Check: quick-py3 (macOS-latest)
- GitHub Check: packaging
- GitHub Check: build-docs
- GitHub Check: min-dep-os (macOS-latest)
- GitHub Check: min-dep-py3 (3.12)
- GitHub Check: min-dep-py3 (3.10)
- GitHub Check: min-dep-os (ubuntu-latest)
- GitHub Check: min-dep-py3 (3.9)
- GitHub Check: min-dep-pytorch (2.6.0)
- GitHub Check: min-dep-py3 (3.11)
- GitHub Check: min-dep-pytorch (2.7.1)
- GitHub Check: min-dep-pytorch (2.5.1)
- GitHub Check: min-dep-pytorch (2.8.0)
- GitHub Check: min-dep-os (windows-latest)
🔇 Additional comments (3)
monai/losses/perceptual.py (3)
26-34: LGTM!Spelling correction from
PercetualNetworkTypetoPerceptualNetworkTypeis correct.
79-79: LGTM!Default parameter correctly uses the renamed
PerceptualNetworkTypeenum.
102-105: LGTM!Validation and error message correctly reference the renamed
PerceptualNetworkTypeenum.
|
Hi @KumoLiu, Conflicts are resolved and pushed. Ready for blossom trigger. Thanks! |
|
/build |
👎 Promotion blocked, new vulnerability foundVulnerability report
|
Signed-off-by: Mohammad Amanour Rahman <[email protected]>
4 participants
Fixes # .
Description
This pull request fixes incorrect formatting and clarifies the documentation
for the
network_typeargument inPerceptualLoss.The docstring now correctly reflects the supported network options
and matches the actual implementation.
Types of changes
./runtests.sh -f -u --net --coverage../runtests.sh --quick --unittests --disttests.make htmlcommand in thedocs/folder.No functional behavior is changed.