feat(membership): recover nested ICC and refuse cross-classified collapse - #117
Conversation
…apse Add a CPU f64 unbalanced ANOVA ICC for nested membership designs and fail closed when the active design is cross-classified or multiple-membership. No new migration (0007 remains owned by #45).
|
Warning Review limit reachedNext included review available in 22 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (12)
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 |
# Conflicts: # CHANGELOG.md
|
Current HEAD |
|
Current-head review refresh for 293067e:
|
|
Exact-head review receipt for PR #117 after normal main synchronization. Current PR head: 8a5a3fa The branch was synchronized with protected main through a normal merge. The only merge conflict was CHANGELOG.md; both the nested ICC capability and naruon loopback capability were retained. Inherited documentation whitespace was corrected so the exact-head diff is clean. Local proof at this exact HEAD: Rust 1.97.1 cargo fmt --check; cargo test --workspace --offline; documentation validation; workspace contract; git diff --check; CodeGraph sync; CodeReviewGraph update against protected main. @cwl-noema-review @opencode-agent Please issue a formal independent review verdict for exactly 8a5a3fa. Review only; do not modify files, push, approve on behalf of another identity, or merge. |
…ed-icc-gate # Conflicts: # CHANGELOG.md # docs/TRACEABILITY.md # docs/adr/0003-relational-event-multiple-membership.md
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| match classify_members(network, instant, outcome_members.iter().copied())? { | ||
| MembershipDesign::Nested => {} | ||
| MembershipDesign::CrossClassified | MembershipDesign::MultipleMembership => { | ||
| return Err(MembershipError::NestedIccInapplicable); | ||
| } | ||
| } | ||
| anova_nested_icc(&groups) |
There was a problem hiding this comment.
📝 Info: Design classification differs in scope between public and ICC paths
classify_membership_design at icc.rs classifies over ALL members in the network, whereas the internal gate in nested_intraclass_correlation (icc.rs) classifies only over the members that supplied outcomes. This is intentional (the ICC only concerns members with outcomes), but it means a network the public classifier reports as CrossClassified can still yield a valid nested ICC if the supplied outcome members happen to each be singly nested. Reviewers relying on the public classifier as a pre-check should be aware the two answers can legitimately diverge.
Was this helpful? React with 👍 or 👎 to provide feedback.
| for outcome in outcomes { | ||
| if !seen.insert(outcome.member_id()) { | ||
| return Err(MembershipError::DuplicateOutcomeMember); | ||
| } | ||
| let active = network.active_memberships_for(outcome.member_id(), instant); | ||
| if active.is_empty() { | ||
| return Err(MembershipError::UnknownOutcomeMember); | ||
| } | ||
| outcome_members.insert(outcome.member_id()); | ||
| let group = active[0].group_id(); | ||
| groups.entry(group).or_default().push(outcome.value()); | ||
| } | ||
| match classify_members(network, instant, outcome_members.iter().copied())? { | ||
| MembershipDesign::Nested => {} | ||
| MembershipDesign::CrossClassified | MembershipDesign::MultipleMembership => { | ||
| return Err(MembershipError::NestedIccInapplicable); | ||
| } | ||
| } | ||
| anova_nested_icc(&groups) | ||
| } |
There was a problem hiding this comment.
📝 Info: Nested ICC gate builds cluster groups before design verification
In nested_intraclass_correlation (icc.rs), the per-group value map is populated using active[0].group_id() for each outcome member before classify_members confirms the design is nested. This is safe: if the design is cross-classified or multiple-membership, the function returns NestedIccInapplicable and the partially-built groups map is discarded. And when the design is Nested, each member has exactly one active membership (single role, single group), so active[0] is unambiguous. No ordering dependence on active_memberships_for's return order affects the result in the nested case.
Was this helpful? React with 👍 or 👎 to provide feedback.
| # ADR 0003 — Relational event ontology and time-varying multiple membership | ||
|
|
||
| **Decision status:** Accepted | ||
| **Implementation maturity:** partial — membership network, event mention/instance separation, and Kish ESS implemented-main; nested ICC with cross-classified/multiple-membership refusal is this increment; full multilevel/MMMC estimators and remaining persistence remain accepted-target |
There was a problem hiding this comment.
🔍 Third Implementation maturity header stacked in ADR 0003
The PR adds another **Implementation maturity:** line to 0003-relational-event-multiple-membership.md on top of two already present, leaving three contradictory maturity headers in one ADR. The same stacking exists in the ADR README rows and appears to be an artifact of the stacked-PR workflow.
Was this helpful? React with 👍 or 👎 to provide feedback.
| let mut groups_by_role: BTreeMap<MembershipRole, BTreeSet<crate::GroupId>> = | ||
| BTreeMap::new(); | ||
| for assignment in active { | ||
| groups_by_role | ||
| .entry(assignment.role()) | ||
| .or_default() | ||
| .insert(assignment.group_id()); | ||
| } | ||
| for groups in groups_by_role.values() { | ||
| if groups.len() >= 2 { | ||
| return Ok(MembershipDesign::MultipleMembership); | ||
| } | ||
| } | ||
| if groups_by_role.len() >= 2 { | ||
| saw_cross = true; | ||
| } | ||
| } | ||
| if !saw_active { | ||
| return Err(MembershipError::InsufficientClusterStructure); | ||
| } | ||
| if saw_cross { | ||
| Ok(MembershipDesign::CrossClassified) | ||
| } else { | ||
| Ok(MembershipDesign::Nested) | ||
| } |
There was a problem hiding this comment.
📝 Info: Nested members in distinct role types treated as nested
classify_members at icc.rs only marks a design cross-classified when a single member holds two or more roles. If outcome members each hold exactly one membership but in different role types (e.g. one Author, one Department), the design is reported Nested and a nested ICC is computed by grouping on group_id. This is a degenerate population-level cross-classification that is not detected because cross-classification is defined per-member. Given the estimator groups purely by group id and each member is singly nested, this is defensible, but it is a subtle definitional choice worth confirming against ADR 0003 intent.
Was this helpful? React with 👍 or 👎 to provide feedback.
| if sum_of_squares_between + sum_of_squares_within == 0.0 { | ||
| return Err(MembershipError::InsufficientClusterStructure); | ||
| } | ||
| let mean_square_between = sum_of_squares_between / (j - 1.0); | ||
| let mean_square_within = sum_of_squares_within / (n - j); | ||
| let harmonic_cluster_size = (n - sum_cluster_size_squared / n) / (j - 1.0); | ||
| let cluster_variance = | ||
| ((mean_square_between - mean_square_within) / harmonic_cluster_size).max(0.0); | ||
| Ok(cluster_variance / (cluster_variance + mean_square_within)) |
There was a problem hiding this comment.
📝 Info: Unbalanced ANOVA n₀ factor matches Snijders–Bosker and recovers balanced ICC exactly
harmonic_cluster_size = (n - sum_cluster_size_squared / n) / (j - 1.0) (icc.rs) is the Snijders–Bosker unbalanced cluster-size factor ñ = (M − Σn_j²/M)/(N−1). For the balanced 4×2 test it reduces to the per-cluster size (2), giving σ²_u=2/3, σ²_e=2, ICC=1/4, matching the contract test. All divisors are guarded: j-1 ≥ 1 (cluster_count ≥ 2), n-j > 0 (sample_size > cluster_count), and the denominator cluster_variance + mean_square_within can only be zero when both SSB and SSW are zero, which is caught by the earlier sum_of_squares_between + sum_of_squares_within == 0.0 guard. No division-by-zero or out-of-range ICC is reachable.
Was this helpful? React with 👍 or 👎 to provide feedback.
1 participant
Summary
ADR 0003 increment from protected
main(40adac9/ #44). Review wait and queued Checks on #45 are local; this slice does not recreatepsychometric_core(#49),compute_backend(#51), or encrypted mapping persistence (waits for #45 /0008).f64unbalanced ANOVA recovers a known nested ICC (1/4) with computed RMSE versus that truth.1; equal cluster means recover ICC0.NestedIccInapplicable.0007remains owned by feat(persistence): retention deletion legal-hold SQL contracts (0007) #45).Claim boundary
Test plan
nested_icc_recovery_contractfailed to compile (E0432/E0599) before the public API existedcargo test -p membership_core --offline --lib --tests --doccargo clippy -p membership_core --all-targets --offline -- -D warningspython3 scripts/check_docstrings.py,check_workspace_contract.py,validate_documentation.pymembership_coreline coverage 188/188 and nightly branch coverage 50/50