fix(fit): seal population label admission - #1522
Conversation
|
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 (2)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe population-label compaction path now validates concrete input carriers and numeric storage before NumPy conversion. It rejects callback-enabled providers, unsupported subclasses, object/string storage, and signed 64-bit overflow. Regression tests cover rejection, boundary values, and mixed-label identity. ChangesPopulation label validation
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to The change tightens population-label input admission while preserving supported normalization behavior, and no actionable merge-blocking risk remains after normal checks and review. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Linked Issues checkExplanation The implementation and focused tests address issue Full details: Out of Scope Changes checkExplanation The changes are limited to population-label admission in _compact_population_labels and focused regression tests. They do not modify estimators, likelihoods, gradients, uncertainty, or Rust numerical ownership.
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
🧪 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 |
Signed-off-by: Seongho Bae <me@seonghobae.me>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
@opencode-agent Please review exact current head All current inline threads are resolved. Focus on callback-free population-label admission; exact supported carrier/scalar identities; signed-64-bit boundary diagnostics; finite nonnegative integer semantics; deterministic sparse-label compaction; compatibility of multigroup, multilevel, inference, and CLI callers; and confirmation that no estimator or likelihood arithmetic moved out of Rust. Return an actual current-head |
Pull request was converted to draft
There was a problem hiding this comment.
Pull request overview
OpenCode reviewed the current-head product diff. Coverage is a separate gate.
Changed files
python/fast_mlsirm/fit.py— Python module behaviortests/test_population_label_int64_roundtrip.py— regression suite
Changed behavior
flowchart LR
PR["PR changed files"] --> Evidence["OpenCode bounded evidence"]
Evidence --> S1["Python: fit.py"]
S1 --> I1["Python module behavior"]
I1 --> R1["Review risk: Python: fit.py"]
R1 --> V1["pytest plus coverage"]
Evidence --> S2["Test: test_population_label_int64_roundtrip.py"]
S2 --> I2["regression suite"]
I2 --> R2["Review risk: Test: test_population_label_int64_roundtrip.py"]
R2 --> V2["targeted test run"]
Findings
No source-backed product finding is synthesized from the coverage gate. A coverage miss belongs in the status comment.
- Head SHA:
8154171e97e9306bf0a6b5e8e02e1ab3f0e814d4 - Workflow run: 33155939122
- Workflow attempt: 1
- Coverage gate:
failure
Review outcome
Coverage is a gate, not the review. This body reviews the changed product files.
Changed-File Evidence Map
flowchart LR
PR["PR changed files"] --> Evidence["OpenCode bounded evidence"]
Evidence --> S1["Python: fit.py"]
S1 --> I1["Python module behavior"]
I1 --> R1["Review risk: Python: fit.py"]
R1 --> V1["pytest plus coverage"]
Evidence --> S2["Test: test_population_label_int64_roundtrip.py"]
S2 --> I2["regression suite"]
I2 --> R2["Review risk: Test: test_population_label_int64_roundtrip.py"]
R2 --> V2["targeted test run"]
OpenCode Review Overview
Coverage evidence did not pass, so approval is blocked. The formal pull-request review is the source-backed diff review, not this status comment. |
|
@cwl-noema-review |
| """A wider real dtype must preserve an exactly representable INT64_MAX label.""" | ||
| if np.finfo(np.longdouble).nmant <= np.finfo(np.float64).nmant: | ||
| pytest.skip("np.longdouble has no additional precision on this platform") | ||
| pytest.fail("np.longdouble has no additional precision on this platform") |
|
Fresh single-writer adoption: |
Closes #1521.
Closes #1530.
Closes #1532.
Scope
Seal population-label admission at the shared
_compact_population_labelsboundary used by multigroup and multilevel estimation without changing likelihoods, estimators, gradients, uncertainty, or Rust numerical ownership.Test-first lineage
15b74bce1c0fe70609d6ec24420aa5a3f06093a3→ GREENb73d76861962c52e989e52c9d8af5658b9c76110: preserve the signed-64-bit range diagnostic belowINT64_MIN.a5d27e6fbe39ad2676d5d38ad29636122b5e8f56→ clean GREEN1430db880ab2c561bb6914eb004e2c6fdb8cf54a:[2**53 + 1, float(2**53)]must remain two population identities rather than collapse through float64 promotion. The unrelated whole-file disturbance introduced on6e34f5d...was neutralized byfa6769a...and is not intended product delta.41800372282fa05d9be60269cbc33e8e2d96cbc0→ GREEN8154171e97e9306bf0a6b5e8e02e1ab3f0e814d4: known wrong-length built-in carriers keep the existing length diagnostic before invalid scalar content can be inspected.f7e33ad8d06075ddc57374a7f5ef77285ce2f268adopts protectedmain@b5a3a0c1057d4b53d7a4bb18e0de69f630c2b45cnon-destructively. Its effective delta remained exactlypython/fast_mlsirm/fit.pyplustests/test_population_label_int64_roundtrip.py.c8ccd650cd25867bd934f23b653ff24ac2ea04c1exposed the reachableULS2PLM/ULSRMfactor normalization being removed from the coverage denominator. GREEN7379a105952f237e8d3d077edef2eb75b45e0757removed that exclusion without changing arithmetic;9fd697b15e3d1ae8e014aa1aa9bbfd803ede2964restored the trailing newline after the whole-file contents write.7163c3b0a727d5cf5d523a66fbd9e43c8db7b46frequires the publicfit()orchestration itself to contain no# pragma: no cover, allowing equivalent refactors while preventing executable public paths from being hidden. GREEN9beb9c7498034ecd14430e4d2e7c5af9166ac77auses the already-validatedn_restarts >= 1invariant to execute restart 0 as the initial best and compare restarts1..n-1, preserving restart order, seeds and objective selection while deleting the unreachable branch. The Devin thread is resolved on the current code.64427a640983e5f849aad16c326697a688b646f6keeps that GREEN and changes the remaining population-label extended-precision portability path frompytest.skip(...)to explicit failure when the host cannot supply wider-than-f64longdoubleprecision. This is test-evidence hardening only and leaves production arithmetic unchanged.Population membership is design evidence for multigroup/multilevel estimation. Python validates and deterministically marshals that evidence; likelihoods, integration, gradients, estimates, uncertainty, and other psychometric arithmetic remain Rust-owned.
Exact-current landing authority — 2026-09-03
main@b5a3a0c1057d4b53d7a4bb18e0de69f630c2b45c;64427a640983e5f849aad16c326697a688b646f6;64427a...is a direct descendant of9beb9c...; the prior body had incorrectly left9beb9c...labeled as the current head after that later non-destructive commit;33756426495has materialized its jobs but remains queued before runner execution; exact-head CodeQL run33756426557has queuedAnalyze (actions)job100651845288, whileAnalyze (python)job100651846463completedskipped. Other repository security workflows are also non-terminal. No predecessor result is transferred.Landing requires all applicable exact-current repository/central CI, security, coverage, package, SBOM/provenance and review gates terminal success on one unchanged head, zero valid unresolved findings, and the qualifying independent approval required by live protected policy. No self-approval, bypass, force update, destructive rebase, gate weakening, no-op retrigger or fabricated/predecessor evidence is authorized.