jc reliability: enforce the no-NaN contract on non-finite input (follow-up to #709)#710
Conversation
The battery's module doc promised `None` "rather than returning NaN", but non-finite input slipped through: spearman(&[1.0, NAN, 2.0], ...) returned Some(1.0) (average_ranks maps NaN to a finite rank, so it never reached the delegated pearson guard) and pearson/cronbach_alpha/icc returned Some(NaN). Add an `all_finite` helper + up-front finite guards on all four public APIs (spearman guards the raw values before ranking), plus trailing result guards so finite-but-overflowing magnitudes (deviations -> +/-inf -> ratio NaN) also return None. Two regression tests cover NaN/+/-inf (incl. the reported reproducer) and 1e308-overflow inputs. Also soften the "ten pillars" aside in the module doc to "registered pillars" (the registry holds 12; a hard count in a doc-comment is a drift magnet). Found post-merge by CodeRabbit (Major) and Codex independently on #709. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Awg6TXocHcwTtc6eGsHcdD
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_5410aa10-7d47-409e-ad5c-ee275e87efc3) |
|
Warning Review limit reached
Next review available in: 39 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the 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 configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: db4889dc76
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let r = sxy / denom; | ||
| // Even with finite inputs, large magnitudes can overflow the squared | ||
| // deviations to ±∞, making the ratio non-finite — reject that too. | ||
| r.is_finite().then_some(r) |
There was a problem hiding this comment.
For large but finite inputs where sxx and syy are still finite but their product overflows, this final is_finite check does not catch the bad result because denom becomes inf and sxy / denom can be a finite 0.0. For example, pearson(&[1e100, -1e100], &[1e100, -1e100]) is perfectly correlated but returns Some(0.0) rather than None, so the new overflow/no-garbage contract is still violated for finite high-magnitude data.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch — confirmed. sxx*syy overflows to ∞ before sqrt, so denom = ∞ and sxy/∞ collapses to a finite-but-wrong 0.0 that the trailing is_finite() guard doesn't reject. Fixed in 0f0bba1 by guarding denom == 0.0 || !denom.is_finite() in pearson (and the same overflowed-denominator pattern in cronbach_alpha's total_var and icc's denom). The overflow test now includes pearson(&[1e100,-1e100],&[1e100,-1e100]) == None.
Generated by Claude Code
The trailing is_finite() guards missed one finite-but-wrong case: when the squared-deviation product (pearson), subject-total variance (cronbach), or mean-square sum (icc) overflows a finite input to +inf, the denominator becomes inf and numerator/inf collapses to a FINITE 0.0. Example flagged by Codex: pearson(&[1e100,-1e100],&[1e100,-1e100]) is perfectly correlated but returned Some(0.0) instead of None. Guard `denom == 0.0 || !denom.is_finite()` in all three ratio computations so an overflowed denominator returns None. The existing result-finiteness guards stay for the non-finite-numerator case. Extend the overflow test with the exact Codex reproducer; note in-test why spearman is immune (it Pearson's the bounded 1..n ranks, which never overflow). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Awg6TXocHcwTtc6eGsHcdD
…::reliability) D-TRI-2 (12-step ↔ 12-family agreement) is NO-GO pre-mint: both 12-vectors are codebook views of the unbuilt minted register (D-TRI-1). Scoped and recorded (E-D-TRI-2-MINT-BLOCKED-1) so no future session re-attempts it. The mint-free cousin: measure whether the three SHIPPED 12-family param tables actually agree — the exact D-TSC-1 "five tables diverged silently" failure mode, using the jc::reliability battery (#709/#710). New probe crates/jc/examples/style_table_agreement.rs (transcribes UNIFIED_STYLES, thinking-engine StyleParams, planner FieldModulation with per-value // SOURCE provenance; verified against source). Runs ICC(2,1)/ICC(3,1)/Cronbach α/pairwise Pearson-Spearman per shared dim in all-12 and 7-explicit modes; pre-registered ICC bands 0.75/0.50; exit-0 (DISTINCT is a valid finding, never a build failure). Measured verdict: - UNIFIED_STYLES ≡ thinking-engine StyleParams: PERFECT (r=ρ=1.0) — M9-dedup confirmed, the two are one table in two homes. - Planner FieldModulation's 7 explicitly-calibrated families: IDENTITY (ICC 0.90-0.98). - ONLY drift: the 5 planner default_modulation fallbacks (convergent/systematic/ divergent/diffuse/peripheral → flat 0.7/6/0.3) drop all-12 ICC to 0.71 (AMBIGUOUS). → TD-PLANNER-STYLE-DEFAULT-DRIFT-1: fill them from the canonical table. Board: EPIPHANIES E-D-TRI-2-MINT-BLOCKED-1, STATUS_BOARD D-TSC-1b, TECH_DEBT, triangle plan §6 annotation, AGENT_LOG. Measurement-only, no behavior change. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Awg6TXocHcwTtc6eGsHcdD
IMPLEMENT — arigraph/community.rs: TripletGraph::communities() — multi-level Louvain modularity over the triplet adjacency (NARS-confidence-weighted edges, self-loops dropped), a carrier method (litmus-compliant), deterministic (BTreeMap-ordered moves + sorted-entity index), 5 inline tests. The STRUCTURAL partition complementing the episodic/family (part_of:is_a) basins AriGraph owns. Louvain core verified standalone before the in-crate build (two-triangle -> 2 comms Q=0.357, clique -> 1, determinism, empty-safe, weighted-cohesion); built against jc/examples/splat_louvain_modularity.rs (parallel-session reference). Leiden refinement pass + PPR/community fusion into retrieval.rs are D-GR-3b. WRITE — plan v1.2: - The base is a MEANING MEASUREMENT: COCA + NARS are co-equal core faculties; agnosticism is scoped to raw data + consumer modelling only (NOT meaning). The 12-byte facet is 6x2x8bit (CLAM), never f32 (f32 is build-time source only); 256 = ranking (needs cosine-replacement), 256^2 = distribution (as accurate as content-blind f32); the 6x(8:8) register is polymorphically part_of:is_a family-identity OR palette256^2 centroid (one register, two lenses) (S3a). - Leiden community synergies: distributional-meaning modes, NARS-truth-weighted (6 axes; the rung-3 layer of the ascent) (S3b). - DocumentID-KV / witness-handle seam: a stable DocumentID keys the consumer KV; the witness/node hold handles only, never raw bytes (S4a, D-GR-6). - New probes: P-COMMUNITY-BASIN-AGREE, P-HIER-LEIDEN-HHTL, P-MEASUREMENT-DETERMINISM, P-PQ-RANK (all via jc::reliability, merged #709/#710). - Meaning-substrate hazard (S7.6): do not reintroduce "keep language out"; the only real constraint is crate-layering (core can't dep the downstream deepnsm crate; use the in-core nsm/ copy). Board hygiene: INTEGRATION_PLANS entry updated to v1.2. Co-Authored-By: Claude <noreply@anthropic.com>
IMPLEMENT — arigraph/community.rs: TripletGraph::communities() — multi-level Louvain modularity over the triplet adjacency (NARS-confidence-weighted edges, self-loops dropped), a carrier method (litmus-compliant), deterministic (BTreeMap-ordered moves + sorted-entity index), 5 inline tests. The STRUCTURAL partition complementing the episodic/family (part_of:is_a) basins AriGraph owns. Louvain core verified standalone before the in-crate build (two-triangle -> 2 comms Q=0.357, clique -> 1, determinism, empty-safe, weighted-cohesion); built against jc/examples/splat_louvain_modularity.rs (parallel-session reference). Leiden refinement pass + PPR/community fusion into retrieval.rs are D-GR-3b. WRITE — plan v1.2: - The base is a MEANING MEASUREMENT: COCA + NARS are co-equal core faculties; agnosticism is scoped to raw data + consumer modelling only (NOT meaning). The 12-byte facet is 6x2x8bit (CLAM), never f32 (f32 is build-time source only); 256 = ranking (needs cosine-replacement), 256^2 = distribution (as accurate as content-blind f32); the 6x(8:8) register is polymorphically part_of:is_a family-identity OR palette256^2 centroid (one register, two lenses) (S3a). - Leiden community synergies: distributional-meaning modes, NARS-truth-weighted (6 axes; the rung-3 layer of the ascent) (S3b). - DocumentID-KV / witness-handle seam: a stable DocumentID keys the consumer KV; the witness/node hold handles only, never raw bytes (S4a, D-GR-6). - New probes: P-COMMUNITY-BASIN-AGREE, P-HIER-LEIDEN-HHTL, P-MEASUREMENT-DETERMINISM, P-PQ-RANK (all via jc::reliability, merged #709/#710). - Meaning-substrate hazard (S7.6): do not reintroduce "keep language out"; the only real constraint is crate-layering (core can't dep the downstream deepnsm crate; use the in-core nsm/ copy). Board hygiene: INTEGRATION_PLANS entry updated to v1.2. Co-Authored-By: Claude <noreply@anthropic.com>
2 participants
What
Follow-up bug fix to #709 (the P5a
jc::reliabilitybattery). The module doc promised all four APIs returnNone"rather than panicking or returningNaN", but non-finite input slipped through the guards.The bug (two reviewers, independently)
spearman(&[1.0, f64::NAN, 2.0], &[1.0, 2.0, 3.0])returnedSome(1.0)—average_ranksmapsNaN(viapartial_cmp → Equal) to an ordinary finite rank, so the NaN never reached the delegatedpearsonguard and produced a finite-but-garbage ρ.pearson/cronbach_alpha/iccreturnedSome(NaN)for non-finite input.This matters because the four measures feed the D-TRI agreement gates; a silent
Some(NaN)/ finite-garbage estimate would poison those gates.The fix
all_finitehelper + up-frontif !all_finite(..) { return None }guard on all four public APIs.spearmanguards the raw values before ranking (not just viapearson), which is the case the guard has to catch.x.is_finite().then_some(x)result guards so finite-but-overflowing magnitudes (deviations → ±∞ → ratioNaN) also returnNone.non_finite_inputs_return_none(NaN/±∞ across all four, incl. the reported reproducer) andoverflowing_large_finite_inputs_return_none_not_nan(1e308 magnitudes).Verification
cargo test --manifest-path crates/jc/Cargo.toml --lib reliability→ 14 passed.cargo test --doc(reliability) → 3 doctests passed.cargo clippy --lib→ no warnings inreliability.rs.Board hygiene
.claude/board/EPIPHANIES.mdprepended withE-JC-BATTERY-NO-NAN-CONTRACT-1(the mergedE-JC-IS-NOT-A-METRIC-BATTERY-1entry is append-only and left untouched).🤖 Generated with Claude Code
https://claude.ai/code/session_01Awg6TXocHcwTtc6eGsHcdD
Generated by Claude Code