Handle constant B-spline mutual information inputs - #9022
Conversation
Signed-off-by: kyinhub <kevinpyin@gmail.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe B-spline kernel now requires more than four bins. Bin-size computation handles constant-intensity inputs without division by zero, and half-precision inputs use float computations for cubic weights. Tests cover invalid bin counts, constant inputs, extreme ranges, finite losses, and finite gradients. Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@monai/losses/image_dissimilarity.py`:
- Around line 233-240: Update the initialization logic around the kernel_type
validation and sigma calculation to reject num_bins=1 for Gaussian kernels
before computing bin-centre spacing, while preserving the existing b-spline
constraint. Add regression coverage confirming that a one-bin Gaussian
configuration raises the expected validation error.
- Around line 293-295: Update the range and bin-size arithmetic in the image
dissimilarity loss to compute intermediate values in float32, apply the
zero-range fallback there, then cast the resulting bin size back to the image
gradient dtype. In
tests/losses/image_dissimilarity/test_global_mutual_information_loss.py, add
small- and large-nonzero-range float16 cases asserting finite loss and
gradients.
In `@tests/losses/image_dissimilarity/test_global_mutual_information_loss.py`:
- Around line 175-179: Extend the constant-image coverage in
test_b_spline_constant_images_are_finite with separate cases for a constant
prediction against a varying target and a varying prediction against a constant
target. Keep the existing finite-loss and gradient assertions, and ensure both
independent zero-intensity-range paths are exercised by the tests.
- Around line 189-193: Update the float16 B-spline tests, including
test_b_spline_constant_half_precision_images_are_finite, to use an
overflow-triggering bin configuration such as num_bins=64 instead of 32.
Preserve the existing assertions and test behavior while ensuring the cubic term
can exceed the float16 range.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: f51b91f4-41e6-4767-9ba0-c9a9ebbf32a0
📒 Files selected for processing (2)
monai/losses/image_dissimilarity.pytests/losses/image_dissimilarity/test_global_mutual_information_loss.py
Signed-off-by: kyinhub <kevinpyin@gmail.com>
|
CI triage: the only failed check is |
|
The only failing check is the macOS full-dep job (89732744347), in the unrelated distributed PersistentDataset test: Gloo cannot resolve the runner .local hostname ( |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/losses/image_dissimilarity/test_global_mutual_information_loss.py (1)
175-177: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument the parameterized test arguments.
Add Google-style
Args:sections forconstant_input,dtype, andmaximum; document_as the parameterized case label or rename it tocase_name.As per path instructions, docstrings should describe each variable, return value, and raised exception in Google-style sections.
Also applies to: 216-217
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/losses/image_dissimilarity/test_global_mutual_information_loss.py` around lines 175 - 177, Update the docstrings for the parameterized tests, including test_b_spline_single_constant_input_is_finite and the additionally referenced test, with Google-style Args sections documenting constant_input, dtype, and maximum. Document the parameterized case-label argument as _ or rename it to case_name, and describe each argument’s purpose without changing test behavior.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@tests/losses/image_dissimilarity/test_global_mutual_information_loss.py`:
- Around line 175-177: Update the docstrings for the parameterized tests,
including test_b_spline_single_constant_input_is_finite and the additionally
referenced test, with Google-style Args sections documenting constant_input,
dtype, and maximum. Document the parameterized case-label argument as _ or
rename it to case_name, and describe each argument’s purpose without changing
test behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 7ea93d41-c1d2-4149-9e0d-16831277d3ca
📒 Files selected for processing (2)
monai/losses/image_dissimilarity.pytests/losses/image_dissimilarity/test_global_mutual_information_loss.py
🚧 Files skipped from review as they are similar to previous changes (1)
- monai/losses/image_dissimilarity.py
Signed-off-by: kyinhub <kevinpyin@gmail.com>
|
Addressed the latest CodeRabbit documentation nit in |
Fixes #9018.
Description
B-spline global mutual information now remains finite when either prediction or target has a constant intensity range and for float16, bfloat16, or float32 inputs with tiny nonzero ranges. Configurations with four or fewer bins are rejected because the implementation reserves two padding bins at each boundary.
The fix performs reduced-precision range, bin-width, and B-spline coordinate arithmetic in float32. Bin widths too small to be represented safely by the compute or input dtype are treated as a degenerate range. This prevents zero-range division, tiny-range underflow, unrepresentable normalization slopes, and cubic-distance overflow while preserving autograd back to the original input.
Types of changes
./runtests.sh -f -u --net --coverage../runtests.sh --quick --unittests --disttests.make htmlcommand in thedocs/folder.Validation
num_bins=64;git diff --checkpass.Scope note
A review also identified the pre-existing Gaussian
num_bins=1configuration issue. It is intentionally not bundled because it is unrelated to the B-spline defect tracked by #9018.Sequencing
This PR remains draft while #9019 is the current MONAI review target.