Skip to content

fix(fit): seal population label admission - #1522

Open
seonghobae wants to merge 19 commits into
mainfrom
fix/population-label-admission-1521
Open

fix(fit): seal population label admission#1522
seonghobae wants to merge 19 commits into
mainfrom
fix/population-label-admission-1521

Conversation

@seonghobae

@seonghobae seonghobae commented Aug 28, 2026

Copy link
Copy Markdown
Contributor

Closes #1521.
Closes #1530.
Closes #1532.

Scope

Seal population-label admission at the shared _compact_population_labels boundary used by multigroup and multilevel estimation without changing likelihoods, estimators, gradients, uncertainty, or Rust numerical ownership.

  • admit only exact one-dimensional NumPy arrays with supported Boolean/integer/floating storage, or exact built-in list/tuple carriers containing exact supported Python/NumPy numeric scalars;
  • reject generic array providers, ndarray/container subclasses, object/string storage, and scalar subclasses before caller conversion protocols can execute;
  • preserve finite non-negative integer validation, signed-int64 boundaries, sparse-ID compaction, float16/extended-precision behavior, and deterministic sorted-unique remapping;
  • normalize trusted scalar sequence elements to package-owned signed-int64 values before NumPy construction so heterogeneous dtype promotion cannot collapse distinct population identities;
  • reject a known built-in carrier length mismatch before scalar scanning, normalization, or NumPy materialization.

Test-first lineage

  • Review RED 15b74bce1c0fe70609d6ec24420aa5a3f06093a3 → GREEN b73d76861962c52e989e52c9d8af5658b9c76110: preserve the signed-64-bit range diagnostic below INT64_MIN.
  • fix(fit): preserve heterogeneous population-label identity before NumPy promotion #1530 RED a5d27e6fbe39ad2676d5d38ad29636122b5e8f56 → clean GREEN 1430db880ab2c561bb6914eb004e2c6fdb8cf54a: [2**53 + 1, float(2**53)] must remain two population identities rather than collapse through float64 promotion. The unrelated whole-file disturbance introduced on 6e34f5d... was neutralized by fa6769a... and is not intended product delta.
  • fix(fit): reject population-label length mismatch before scalar traversal #1532 RED 41800372282fa05d9be60269cbc33e8e2d96cbc0 → GREEN 8154171e97e9306bf0a6b5e8e02e1ab3f0e814d4: known wrong-length built-in carriers keep the existing length diagnostic before invalid scalar content can be inspected.
  • Reconciliation f7e33ad8d06075ddc57374a7f5ef77285ce2f268 adopts protected main@b5a3a0c1057d4b53d7a4bb18e0de69f630c2b45c non-destructively. Its effective delta remained exactly python/fast_mlsirm/fit.py plus tests/test_population_label_int64_roundtrip.py.
  • Coverage-governance RED c8ccd650cd25867bd934f23b653ff24ac2ea04c1 exposed the reachable ULS2PLM/ULSRM factor normalization being removed from the coverage denominator. GREEN 7379a105952f237e8d3d077edef2eb75b45e0757 removed that exclusion without changing arithmetic; 9fd697b15e3d1ae8e014aa1aa9bbfd803ede2964 restored the trailing newline after the whole-file contents write.
  • Devin review then identified that the first regression overfit one exact assignment spelling. Generalized RED 7163c3b0a727d5cf5d523a66fbd9e43c8db7b46f requires the public fit() orchestration itself to contain no # pragma: no cover, allowing equivalent refactors while preventing executable public paths from being hidden. GREEN 9beb9c7498034ecd14430e4d2e7c5af9166ac77a uses the already-validated n_restarts >= 1 invariant to execute restart 0 as the initial best and compare restarts 1..n-1, preserving restart order, seeds and objective selection while deleting the unreachable branch. The Devin thread is resolved on the current code.
  • Current descendant 64427a640983e5f849aad16c326697a688b646f6 keeps that GREEN and changes the remaining population-label extended-precision portability path from pytest.skip(...) to explicit failure when the host cannot supply wider-than-f64 longdouble precision. 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

  • protected base: main@b5a3a0c1057d4b53d7a4bb18e0de69f630c2b45c;
  • exact current head: 64427a640983e5f849aad16c326697a688b646f6;
  • GitHub reports the PR open, Ready and mergeable;
  • 64427a... is a direct descendant of 9beb9c...; the prior body had incorrectly left 9beb9c... labeled as the current head after that later non-destructive commit;
  • exact-head CI run 33756426495 has materialized its jobs but remains queued before runner execution; exact-head CodeQL run 33756426557 has queued Analyze (actions) job 100651845288, while Analyze (python) job 100651846463 completed skipped. 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.

@coderabbitai

coderabbitai Bot commented Aug 28, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 7ba9e1fe-8eed-4e14-af43-84d862539fa2

📥 Commits

Reviewing files that changed from the base of the PR and between 4562770 and a5d27e6.

📒 Files selected for processing (2)
  • python/fast_mlsirm/fit.py
  • tests/test_population_label_int64_roundtrip.py

Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review.


📝 Walkthrough

Walkthrough

The 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.

Changes

Population label validation

Layer / File(s) Summary
Validate and compact population labels
python/fast_mlsirm/fit.py
The compaction logic allows trusted concrete scalar types, validates arrays and sequences, rejects conversion hooks and unsupported storage, and enforces signed 64-bit limits.
Cover rejected and preserved label cases
tests/test_population_label_int64_roundtrip.py
Regression tests cover hostile providers, subclasses, unsupported storage, signed 64-bit boundaries, and mixed large integer and float labels.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Merge Risk: ⚪ Minimal · up to a5d27

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)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 76.47% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 17 functions across 2 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed The implementation and focused tests address issue #1521. They validate callback-free exact carriers and supported scalar types, reject providers, subclasses, object/string storage, and preserve exist…
Out of Scope Changes check ✅ Passed 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 …
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly identifies the main change: tightening population-label admission in fit.py. It is concise and specific.
Full details: Linked Issues check

Explanation

The implementation and focused tests address issue #1521. They validate callback-free exact carriers and supported scalar types, reject providers, subclasses, object/string storage, and preserve existing label validation, int64 boundaries, and deterministic compaction behavior.

Full details: Out of Scope Changes check

Explanation

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.

  • Fix all pre-merge checks with AI
✨ Finishing Touches 💡 2
📝 Generate docstrings 💡
  • Create stacked PR
  • Commit on current branch
🛠️ Fix failing CI checks 💡
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/population-label-admission-1521

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.

Signed-off-by: Seongho Bae <me@seonghobae.me>
@seonghobae
seonghobae marked this pull request as ready for review August 28, 2026 02:24
@chatgpt-codex-connector

Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.
To continue using code reviews, you can upgrade your account or add credits to your account and enable them for code reviews in your settings.

devin-ai-integration[bot]

This comment was marked as resolved.

@seonghobae
seonghobae enabled auto-merge (squash) August 28, 2026 04:01

Copy link
Copy Markdown
Contributor Author

@opencode-agent
@cwl-noema-review

Please review exact current head b73d76861962c52e989e52c9d8af5658b9c76110 against protected main@45627700c26c29bca150896a9519a9b7426acb56. Review only: do not update the branch, merge, or bypass protection.

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 APPROVED or CHANGES_REQUESTED review rather than fallback evidence.

devin-ai-integration[bot]

This comment was marked as resolved.

@seonghobae
seonghobae marked this pull request as draft August 28, 2026 07:36
auto-merge was automatically disabled August 28, 2026 07:36

Pull request was converted to draft

devin-ai-integration[bot]

This comment was marked as resolved.

@seonghobae
seonghobae marked this pull request as ready for review August 28, 2026 08:04

@opencode-agent opencode-agent Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

OpenCode reviewed the current-head product diff. Coverage is a separate gate.

Changed files

  • python/fast_mlsirm/fit.py — Python module behavior
  • tests/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"]
Loading

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"]
Loading

@opencode-agent

opencode-agent Bot commented Aug 28, 2026

Copy link
Copy Markdown
Contributor

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.

Copy link
Copy Markdown
Contributor Author

@opencode-agent

@seonghobae
seonghobae enabled auto-merge (squash) September 1, 2026 03:37

Copy link
Copy Markdown
Contributor Author

@cwl-noema-review

@opencode-agent
opencode-agent Bot disabled auto-merge September 1, 2026 10:20
devin-ai-integration[bot]

This comment was marked as resolved.

@devin-ai-integration devin-ai-integration Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Devin Review found 1 new potential issue.

Devin 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")

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔍 Windows test suite always fails

On Windows, NumPy long double equals float64 precision, so pytest.fail always fails this test. The published Windows platform cannot pass the full suite.

Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

Copy link
Copy Markdown
Contributor Author

Fresh single-writer adoption: tests/test_population_label_int64_roundtrip.py now owns the extended-precision non-execution repair directly. Exact head 64427a640983e5f849aad16c326697a688b646f6 changes the platform-without-wider-longdouble path from passing pytest.skip to explicit pytest.fail; population-label production arithmetic/admission is otherwise unchanged. #1733 is Draft and treats its mirrored copy as temporary dependency evidence until this canonical lane integrates. All predecessor checks/reviews are stale after this head move.

@seonghobae seonghobae added bug Something isn't working priority: medium Normal-priority or P2 work type: bug Defect or incorrect behavior labels Sep 7, 2026 — with ChatGPT Codex Connector
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working priority: medium Normal-priority or P2 work type: bug Defect or incorrect behavior

Projects

None yet

1 participant