Skip to content

refactor(inference): move covariance arithmetic to Rust - #647

Merged
seonghobae merged 3 commits into
mainfrom
refactor/inference-rust-vcov-625
Aug 11, 2026
Merged

refactor(inference): move covariance arithmetic to Rust#647
seonghobae merged 3 commits into
mainfrom
refactor/inference-rust-vcov-625

Conversation

@seonghobae

Copy link
Copy Markdown
Contributor

Production numerical-ownership gap

Advances #625 with a bounded disjoint first slice. Protected main a7be9eb8ca0b807364173f8471fc300e3cf9350f still performs covariance inversion/pseudoinversion and standard-error extraction in Python/NumPy. Those operations directly determine reported parameter uncertainty and therefore belong to the Rust production numerical owner.

Intentional RED

Exact fail-first head 00ef30eea93352bcd4c39026aefcb70a590bd33b, directly based on protected main a7be9eb8ca0b807364173f8471fc300e3cf9350f.

The new production-boundary tests install sentinel compiled-core functions and require:

  • vcov_from_hessian() to delegate inverse/pseudoinverse arithmetic to _core.vcov_from_hessian; and
  • standard_errors_from_vcov() to delegate covariance-diagonal clamping/square-root arithmetic to _core.standard_errors_from_vcov.

Current protected-main functions use np.linalg.inv/np.linalg.pinv and np.sqrt(np.maximum(np.diag(...))), so the sentinel call-count/result assertions are expected to be the only REDs. Setup/import/fixture/changelog failure is not valid RED.

Planned minimum GREEN after RED proof

  1. Implement the current covariance and SE semantics in mlsirm-core: square-matrix validation at the binding boundary, inverse with Moore-Penrose fallback using the existing rcond contract, symmetric returned covariance, and non-negative-diagonal square-root SE behavior.
  2. Expose thin PyO3 vcov_from_hessian and standard_errors_from_vcov entrypoints; Python validates/marshals/reshapes only and fails closed if compiled capability is absent.
  3. Preserve exact public shape/dtype/error behavior for ordinary, singular, near-singular, indefinite and negative-diagonal inputs unless a separately reviewed scientific correction is justified.
  4. Add direct Rust↔Python matrix/field parity, symmetry/pseudoinverse properties, caller-array immutability and stable non-reflective malformed-input tests. Add uncertainty/recovery/coverage evidence where these outputs support parameter-uncertainty claims; algebraic parity alone is insufficient for release claims.
  5. Do not widen this slice into observed_information() finite-difference Hessian construction or second_order_test() eigenanalysis; those remain separately sequenced within architecture: migrate observed-information and covariance numerics from Python to Rust #625.
  6. Add complete rustdoc/docstrings, authoritative changelog/APA 7 doctoring as applicable, exact changed-production statement/branch coverage, then full Python/Rust/PyO3/package/GPU-existing/fuzz/Security/SAST evidence on one unchanged final head.
  7. Canonical Architecture/TRD/ADR/traceability changes remain owned by docs: establish canonical product and architecture baseline #604/docs: close canonical documentation fitness gaps without parallel authority #621.

Writer boundary

This branch changes only inference covariance/SE ownership and is disjoint from active backend #645, fitstats #646, fit.py #642, config.py #640, serving #638, validation #635 and canonical docs #604. Keep Draft until RED→GREEN history, exact-head review and repository policy are satisfied.

@coderabbitai

coderabbitai Bot commented Aug 10, 2026

Copy link
Copy Markdown

Warning

Review limit reached

@seonghobae, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 8 minutes

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 03d8b35a-9895-455a-86c7-7c02ecf0bd96

📥 Commits

Reviewing files that changed from the base of the PR and between 640de7d and 9b9db00.

📒 Files selected for processing (7)
  • crates/fast-mlsirm-py/src/lib.rs
  • crates/mlsirm-core/src/inference.rs
  • crates/mlsirm-core/src/lib.rs
  • docs/changelog.d/647-inference-rust-vcov.md
  • docs/doctoring/inference_rust_vcov_ownership.md
  • python/fast_mlsirm/inference.py
  • tests/test_inference_rust_vcov_ownership.py

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

Copy link
Copy Markdown
Contributor Author

@opencode-agent address

Exact bounded RED→GREEN implementation handoff for issue #625 / Draft #647. Immediately before any write, refetch exact PR head, independently resolve live protected main, inspect comments/reviews, and refetch every target blob/ref. Proceed only if #647 is still exact head 00ef30eea93352bcd4c39026aefcb70a590bd33b, live protected main is still a7be9eb8ca0b807364173f8471fc300e3cf9350f, and no other writer targets this branch or the same inference/PyO3/Rust paths; otherwise abort writes.

The intended production-boundary RED is proven on exact CI run 31358334846, Python job 93362107822, synthetic merge 69261731024c50a620bad07642b5af4c7bab9b1b: setup, locked dependencies, editable Rust/PyO3 build and Rust-primary backend verification succeeded; Rust, GPU, fuzz, package/reinstall/release acceptance, Security Scan and SAST are GREEN; Python completed 2 failed, 2948 passed, 2 skipped. The only failures are test_vcov_from_hessian_delegates_numerical_result_to_rust and test_standard_errors_from_vcov_delegate_to_rust, each because the sentinel compiled-core function is called zero times. The marginal _log_sigmoid warning is unrelated #583/#564.

Implement only the minimum root-cause fix for this first #625 slice. Move covariance inverse/pseudoinverse and standard-error diagonal reduction/square-root arithmetic into mlsirm-core, expose thin canonical PyO3 vcov_from_hessian and standard_errors_from_vcov bindings, and reduce Python to bounded validation/marshalling/shape restoration/result transport. Missing compiled capability must fail closed; do not retain ordinary NumPy production fallback. Preserve current contract for the rcond inverse→Moore–Penrose fallback, symmetry of returned covariance, non-negative diagonal handling, shape/dtype/error behavior, caller immutability, singular/near-singular/indefinite inputs, unless a separately evidenced scientific correction is required. Add direct Rust↔Python parity/property tests plus uncertainty/recovery/coverage evidence appropriate to claims made by these outputs; algebraic parity alone is not release evidence. Do not widen into observed_information() finite-difference Hessian construction or second_order_test() eigenanalysis; those remain later #625 slices.

Add complete rustdoc/docstrings, APA-7 doctoring grounded in authoritative numerical/statistical sources as applicable, an authoritative changelog fragment, then render/check managed CHANGELOG after GREEN. Run focused tests before full applicable Python/Rust/PyO3/package/GPU/fuzz/security/SAST gates on one unchanged final head. Do not touch backend #645, fitstats #646, fit.py #642, config #640, serving #638, validation #635, canonical docs #604/#621, dependencies/workflows/version/release. Commit coherent changes, leave Draft, then stop source writes for exact-head maintainer review/Ready/merge.

Copy link
Copy Markdown
Contributor Author

@jules address

Fallback sole branch writer for exact Draft #647 head 00ef30eea93352bcd4c39026aefcb70a590bd33b over protected main a7be9eb8ca0b807364173f8471fc300e3cf9350f. Prior OpenCode handoff 5236302953 has no receipt/reaction and no source movement; treat it as inactive for this bounded RED→GREEN implementation only. Immediately refetch head/main/comments/reviews and every target blob/ref; abort if either ref moves or another writer appears on this branch or the inference/PyO3/Rust paths. Do not create another branch or widen scope.

The intended RED is exact: CI 31358334846 checked out synthetic merge 69261731024c50a620bad07642b5af4c7bab9b1b, reached production, and ended 2 failed, 2948 passed, 2 skipped. The only failures are vcov_from_hessian() and standard_errors_from_vcov() ownership sentinel call-count tests; Rust/package/GPU/fuzz/Security/SAST are green. _log_sigmoid remains unrelated #583/#564.

Implement only this first #625 slice: move covariance inverse/pseudoinverse and covariance-diagonal SE arithmetic into mlsirm-core; expose thin canonical PyO3 vcov_from_hessian and standard_errors_from_vcov; leave Python with bounded validation/marshalling/shape restoration/transport and fail closed if the compiled capability is absent. Preserve current rcond inverse→Moore–Penrose fallback, symmetric covariance result, non-negative diagonal SE semantics, shape/dtype/errors/caller immutability and singular/near-singular/indefinite behavior unless a separately proven defect exists. Add direct Rust↔Python parity/properties plus realistic uncertainty/recovery/coverage evidence appropriate to this output surface, complete rustdoc/docstrings, APA 7 doctoring, and an authoritative changelog fragment. Run focused tests first, then applicable Python/Rust/PyO3/package/GPU/fuzz gates; render/check managed CHANGELOG after GREEN; run git diff --check. Do not widen into observed_information, second_order_test, backend #645, fitstats #646, fit #642, config #640, serving #638, validation #635, canonical docs #604/#621, dependencies/workflows/version/release. Commit one coherent verified series, leave Draft, then stop writing; final exact-head review/Ready/merge belongs to the maintainer loop.

Copy link
Copy Markdown
Contributor Author

@jules address

Supersede the inactive OpenCode implementation handoff 5236302953 for Draft #647 only. Final pre-write guard is mandatory: proceed only if source is still 00ef30eea93352bcd4c39026aefcb70a590bd33b, protected main is still a7be9eb8ca0b807364173f8471fc300e3cf9350f, the prior OpenCode handoff still has no receipt/reaction/source movement, and no newer writer targets the inference/PyO3/Rust files; otherwise abort rather than race.

The intended production-boundary RED is already exact: CI 31358334846, Python job 93362107822, synthetic merge 69261731024c50a620bad07642b5af4c7bab9b1b completed 2 failed, 2948 passed, 2 skipped; only the two sentinel ownership tests failed because Python still owns covariance inverse/pseudoinverse and SE diagonal/sqrt arithmetic. Rust, package/reinstall/release acceptance, GPU, fuzz, Security and SAST were GREEN.

Implement only the first bounded #625 slice: move vcov_from_hessian inverse→Moore–Penrose fallback/symmetrization and standard_errors_from_vcov non-negative diagonal/sqrt arithmetic into mlsirm-core; expose thin canonical PyO3 entrypoints; reduce Python to bounded validation/marshalling/shape restoration and fail closed if compiled capability is missing. Preserve current rcond, shape/dtype/error, singular/near-singular/indefinite, negative-diagonal and caller-immutability semantics unless a separately evidenced scientific defect is found. Add direct Rust↔Python parity/property tests and uncertainty evidence appropriate to claims; do not widen into observed_information() Hessian construction or second_order_test() eigenanalysis.

After focused GREEN, add complete rustdoc/docstrings, APA-7 doctoring and an authoritative changelog fragment; render/check managed CHANGELOG; run focused then full applicable Python/Rust/PyO3/package/GPU/fuzz/Security/SAST gates on one unchanged head. Do not touch backend #645, fitstats #646, fit.py #642, config #640, serving #638, validation #635, canonical #604/#621 docs, dependencies/workflows/version/release. Keep Draft and stop branch writes after a coherent verified update; review/Ready/merge return to this maintainer loop.

Copy link
Copy Markdown
Contributor Author

@opencode-agent address

Fresh maintainer-loop takeover for exact Draft #647 head 00ef30eea93352bcd4c39026aefcb70a590bd33b over protected main a7be9eb8ca0b807364173f8471fc300e3cf9350f. The prior exact RED→GREEN handoff 5236302953 has remained unacknowledged with zero reactions and no source movement for many hours; treat it as stale predecessor writer evidence. Immediately before any write, refetch head/main/comments/reviews and every inference/Rust/PyO3 target blob; abort if refs or writer state changed.

The exact production RED is already established: CI 31358334846, Python job 93362107822, synthetic merge 69261731024c50a620bad07642b5af4c7bab9b1b finished 2 failed, 2948 passed, 2 skipped. The only failures are the sentinel delegation tests for vcov_from_hessian() and standard_errors_from_vcov(), proving NumPy currently owns covariance inversion/pseudoinversion and SE diagonal reduction. Rust/GPU/fuzz/package/reinstall/release acceptance/Security/SAST were green; _log_sigmoid is unrelated #583/#564.

Implement only this first bounded #625 ownership slice: move current covariance inverse→Moore–Penrose fallback and standard-error diagonal clamping/square-root arithmetic into mlsirm-core, expose thin canonical PyO3 vcov_from_hessian / standard_errors_from_vcov, and leave Python to bounded validation/marshalling/shape restoration/result transport. Missing compiled capability must fail closed. Preserve current rcond, symmetry, non-negative diagonal behavior, shape/dtype/errors, singular/near-singular/indefinite handling and caller immutability unless a separately evidenced scientific defect appears. Retained Python arithmetic may remain only as explicit parity/reference evidence, never ordinary public production fallback. Add Rust↔Python property/parity tests plus uncertainty/recovery/coverage evidence appropriate to reported SE claims, complete rustdoc/docstrings, APA 7 doctoring and authoritative changelog material after GREEN, then render/check managed CHANGELOG.md.

Run focused tests first, meaningful changed-production statement/branch coverage/docstrings, then complete applicable Python/Rust/PyO3/package/reinstall/GPU/fuzz/Security/SAST on one unchanged final head. Keep Draft and stop source writes after one coherent verified update; final review/Ready/merge returns to the maintainer loop. Do not widen into finite-difference Hessian construction, second-order eigenanalysis, backend #645, fitstats #646, fit #642, config #640, serving #638, validation #635, dependencies/workflows/version/release, or canonical #604/#621 docs.

Copy link
Copy Markdown
Contributor Author

@opencode-agent address

Fresh current-main reconciliation + GREEN handoff for Draft #647. The previous handoff was explicitly bound to protected main a7be9eb8... and is stale after protected-main movement. Immediately refetch #647 exact head, live main, comments/reviews and target blobs; proceed only if source remains 00ef30eea93352bcd4c39026aefcb70a590bd33b and live main is still compatible with 56967deceafeb7ecd0ff08a98f196282a6ec81a3. Abort if another writer appears or either ref moves.

Fresh compare is diverged, ahead 1 / behind 151, merge base a7be9eb8ca0b807364173f8471fc300e3cf9350f; the only unique path is tests/test_inference_rust_vcov_ownership.py. Reconcile current protected main non-destructively first, preserving all accepted backend/serving/CAT/mixed/node-rule/compatibility/release-evidence changes and retaining this fail-first test as the unique branch slice. Prior RED remains historical evidence only.

Then implement the minimum first #625 GREEN only: Rust-owned covariance inverse/pseudoinverse and standard-error diagonal reduction/square root, thin PyO3 vcov_from_hessian and standard_errors_from_vcov, Python bounded validation/marshalling/result transport only, fail closed if compiled capability missing. Preserve current rcond, inverse→Moore–Penrose fallback, covariance symmetry, non-negative diagonal SE semantics, shape/dtype/error behavior and caller immutability. Add Rust↔public parity/property and uncertainty/recovery/coverage evidence appropriate to the public uncertainty claim. Do not widen into observed-information finite-difference Hessian or second-order eigenanalysis. Add complete rustdoc/docstrings, APA 7 doctoring/changelog, exact changed coverage and full same-head gates; keep Draft for final review/merge. Do not touch dependencies/workflows/version/release or canonical #604 docs.

@seonghobae
seonghobae force-pushed the refactor/inference-rust-vcov-625 branch from 00ef30e to b2d8eec Compare August 11, 2026 08:47
@seonghobae
seonghobae marked this pull request as ready for review August 11, 2026 08:47

Copy link
Copy Markdown
Contributor Author

@opencode-agent address

Exact-current scientific/parity defect on #647 head b2d8eec40b769b1da1aea9f05596b80c2ff5be98, protected main d2301dccb7d39a794d08cc96ea8c858b9ff49125, Rust inference blob 971e9c11ab37e65f5c14e9aa7523b3021f93fb38. Refetch all three plus writer state immediately before any source write and abort if they move.

mlsirm_core::inference::standard_errors_from_vcov() currently returns 0.0 for every diagonal value that is not both finite and positive:

out[i] = if d.is_finite() && d > 0.0 { d.sqrt() } else { 0.0 };

That is not the protected-main contract np.sqrt(np.maximum(np.diag(matrix), 0.0)): negative finite values map to zero, but NaN remains NaN and +Inf remains +Inf. The current Rust migration therefore silently converts undefined/infinite uncertainty into zero standard error, a materially dangerous confidence inflation. It also contradicts the PR's explicit requirement to preserve exact non-finite behavior unless separately reviewed.

Keep #647 unmergeable until a fail-first regression pins this boundary and GREEN either (a) preserves exact existing semantics (NaN -> NaN, +Inf -> +Inf, finite negative -> 0), or (b) deliberately fails closed on non-finite covariance with a separate documented scientific/API decision; never coerce non-finite uncertainty to zero. Add direct Rust↔Python parity for finite positive/zero/negative/NaN/+Inf diagonals and ensure downstream reports cannot interpret invalid uncertainty as precise certainty.

Also revalidate vcov_from_hessian on non-finite matrices: the new Gauss-Jordan path currently has no finite-entry preflight, so NaN pivots can propagate through Some(inv) rather than taking a controlled error/pseudoinverse path. Preserve current public behavior exactly or fail closed consistently, but do not create backend-dependent non-finite semantics.

Run focused ownership/parity/uncertainty tests and all same-head gates after repair. Do not widen into observed-information Hessian construction or second-order eigenanalysis.

Copy link
Copy Markdown
Contributor Author

@opencode-agent address

The previously reported invalid-uncertainty defect remains present on exact current head 01f7f9c5638b6dfe28d837b19a925d9f8d2c5c4a over protected main d2301dccb7d39a794d08cc96ea8c858b9ff49125; exact Rust blob is still 971e9c11ab37e65f5c14e9aa7523b3021f93fb38. Refetch head/base/blob/comments immediately before writing and abort if any moves or another writer appears.

standard_errors_from_vcov() still maps every non-finite diagonal to 0.0, so NaN/+Inf uncertainty becomes false zero SE. This still contradicts protected-main np.sqrt(np.maximum(diag, 0.0)), where NaN remains NaN and +Inf remains +Inf. vcov_from_hessian() also still lacks finite-entry preflight. The current head therefore remains scientifically unsafe to merge.

Execute the already-scoped repair now on this exact head: add fail-first Rust↔Python parity for finite positive/zero/negative/NaN/+Inf diagonals plus non-finite Hessian input; GREEN must either preserve existing non-finite semantics exactly or fail closed consistently with a documented package error, but never coerce invalid/infinite uncertainty to zero. Ensure reports/downstream callers cannot receive false precision. Run focused inference ownership/parity/coverage tests and all same-head gates. Do not widen into observed-information Hessian construction or second-order eigenanalysis.

Add mlsirm_core::inference for Hessian inversion / pseudoinverse and
standard-error extraction; Python wrappers marshal only.
@seonghobae
seonghobae force-pushed the refactor/inference-rust-vcov-625 branch from 01f7f9c to 363fc12 Compare August 11, 2026 09:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant