fix(inference): preserve non-finite uncertainty after Rust SE migration - #725
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThe inference APIs now reject non-finite Hessian entries and preserve non-finite covariance diagonal values during standard-error conversion. Rust and Python tests and documentation define the behavior. Changelog entries record these and other release changes. ChangesInference uncertainty handling
Release changelog updates
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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)
docs/doctoring/inference_nonfinite_uncertainty.md (1)
13-18: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAdd source links and source-specific rationale.
This change defines an interpretation-facing uncertainty contract. The document cites two sources, but it does not provide stable links or explain how either source supports preserving non-finite covariance diagonals and clamping finite non-positive diagonals. Add those links and a short paper-basis summary before merging.
As per coding guidelines, substantive feature or process PRs must include relevant academic paper PDFs with full citations when redistribution is permissible; otherwise include a citation, link, and summary. Estimator and interpretation-facing output changes also require relevant literature research and a paper basis in the plan.
🤖 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 `@docs/doctoring/inference_nonfinite_uncertainty.md` around lines 13 - 18, Add stable source links for both references in the References section and add a concise source-specific rationale explaining how each supports preserving non-finite covariance diagonals and clamping finite non-positive diagonals. Include the required literature basis using full citations plus PDFs when redistribution is permitted, or otherwise provide each citation, link, and summary.Source: Coding guidelines
🧹 Nitpick comments (1)
tests/test_inference_nonfinite_uncertainty.py (1)
12-20: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winCover negative infinity in the regression tests. The implementation and documentation preserve signed infinities, but the tests cover only
NaNand+∞. Add negative-infinity cases to keep the Python and Rust contracts symmetric.
tests/test_inference_nonfinite_uncertainty.py#L12-L20: include-np.infand assertnp.isneginf(result[5]).tests/test_inference_nonfinite_uncertainty.py#L23-L32: assert negative-infinity preservation for both the direct Rust and public results.crates/mlsirm-core/src/inference.rs#L216-L231: addf64::NEG_INFINITYand assertis_infinite() && is_sign_negative().🤖 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/test_inference_nonfinite_uncertainty.py` around lines 12 - 20, Extend negative-infinity coverage for standard errors: in tests/test_inference_nonfinite_uncertainty.py lines 12-20, add -np.inf to the covariance input and assert result[5] is negative infinity; in lines 23-32, assert negative-infinity preservation for both direct Rust and public results; in crates/mlsirm-core/src/inference.rs lines 216-231, add f64::NEG_INFINITY and verify it remains infinite with a negative sign.
🤖 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.
Outside diff comments:
In `@docs/doctoring/inference_nonfinite_uncertainty.md`:
- Around line 13-18: Add stable source links for both references in the
References section and add a concise source-specific rationale explaining how
each supports preserving non-finite covariance diagonals and clamping finite
non-positive diagonals. Include the required literature basis using full
citations plus PDFs when redistribution is permitted, or otherwise provide each
citation, link, and summary.
---
Nitpick comments:
In `@tests/test_inference_nonfinite_uncertainty.py`:
- Around line 12-20: Extend negative-infinity coverage for standard errors: in
tests/test_inference_nonfinite_uncertainty.py lines 12-20, add -np.inf to the
covariance input and assert result[5] is negative infinity; in lines 23-32,
assert negative-infinity preservation for both direct Rust and public results;
in crates/mlsirm-core/src/inference.rs lines 216-231, add f64::NEG_INFINITY and
verify it remains infinite with a negative sign.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 887cac9d-0ebf-4045-970c-644d7b9eb31c
📒 Files selected for processing (6)
CHANGELOG.mdcrates/mlsirm-core/src/inference.rsdocs/changelog.d/722-inference-nonfinite-uncertainty.mddocs/doctoring/inference_nonfinite_uncertainty.mdpython/fast_mlsirm/inference.pytests/test_inference_nonfinite_uncertainty.py
Stop converting NaN/infinite covariance diagonals into false zero uncertainty. Fail closed when observed information contains non-finite entries so uncontrolled covariance artifacts cannot be published.
a0665a3 to
aa931ad
Compare
1 participant
Why
Advances #722. After #647,
standard_errors_from_vcovclamped every non-positive or non-finite diagonal to0.0, reporting perfect certainty for undefined or unbounded variance. Non-finite Hessians could also produce uncontrolled covariance artifacts.What
NaNand infinite diagonal uncertainty; clamp only finite non-positive diagonals to zero.Supersedes draft #722 once green.
Summary by CodeRabbit
Bug Fixes
NaNand infinite uncertainty values in standard-error results.Documentation