feat: name leftover-map explained share on leftover pairs (v2.12.28) - #539
feat: name leftover-map explained share on leftover pairs (v2.12.28)#539seonghobae wants to merge 15 commits into
Conversation
Persist explained leftover share e = R̂_c² / R̃² of centered leftover on leftover post–criterion pairs (ADR 0184). Two-axis reconstruction R̂_c stays internal. A reconstructed rank-1 cell stores share 1 even when mean(R) ≠ 0. A 0/0 origin cell stores 0. Fallback omits the share. Do not persist leftover-map unexplained leftover share s or reconstruction R̂.
|
Warning Review limit reachedNext included review available in 40 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 (26)
📝 WalkthroughWalkthrough잔여 쌍 계산이 centered leftover 기반 설명 공유율 Changes잔여 맵 설명 공유
Estimated code review effort: 4 (Complex) | ~45 minutes Merge Risk: 🟡 Moderate · up to The PR adds persisted explained-share values and new badges, but valid small rank-1 cells can display 0% and very large inputs can produce invalid values, leading to incorrect user-facing explanations. Merge should wait for the calculation fix and regression coverage. Sequence Diagram(s)sequenceDiagram
participant Seed as seed_demo_data.py
participant Report as persist_period_report
participant Database as report_leftover_pair
participant API as fetch_period_reports
participant Browser as App.tsx
Seed->>Report: leftover pair 및 explained share 전달
Report->>Database: leftover_map_explained_share 저장
Database-->>API: 저장된 leftover pair 조회
API-->>Browser: leftover_map_explained_share 포함 응답
Browser->>Browser: 공유율·거리·criterion 표시
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Squaring before the 1e-12 comparison made the effective singular threshold sqrt(1e-12) = 1e-6, collapsing small-but-valid rank-1 cells to share 0, and could overflow large finite inputs to inf. Compare absolute values first, compute the ratio once, and persist only a finite, non-negative result. Adds regression tests for both edges (devin review thread PRRT_kwDOT22WIM6bi4hz).
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
… share
- floor the explained share on absolute magnitudes so a small-but-valid
rank-1 cell keeps its share and large finite inputs cannot overflow to
inf; add regression tests for both edges
- show the centered-reconstruction subscript (R-hat_c squared over
R-tilde squared) in the share badge so UI notation matches the ADR
definition; update badge tests
- build the pair aria-label from the localized kind label so screen
readers stop reading raw closest/farthest enums, mirroring visible text
- smooth Korean next-action copy ({value}만큼, 주효과 이후 가장 가까운/먼
기준) and update i18n expectations
- align ADR 0049's screen-reader contract with the criterion-bearing
label this increment ships
devin review threads on PR #539.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Resolves 23 conflicts against a main that had moved well past this branch's base, including several already-landed leftover-map amendments (ADR 0119/0162/0163/0164) this branch predated. Key resolutions: - lineageweave/leftover_pairs.py: combined ADR 0184's explained-share computation with the already-landed observed/expected/rank tracking -- LeftoverPair now carries all of it. Both _candidate_row and _pair_from_candidate needed their signatures widened to a 7-element candidate row. - tests/test_leftover_pairs.py: two tests (rank-three explained-share, rank-four map-vs-full distance) shared identical 4x4 matrix setup by coincidence but tested different things -- split into two complete, independent test functions instead of forcing one merge. Found and fixed a real pre-existing bug in the rank-three test along the way: it asserted leftover_distance equals the *full*-rank Gabriel distance, which directly contradicts the already-established (and now doubly-tested) 2-axis-map behavior documented in the module's own docstring. - frontend/src/App.tsx: took main's <LeftoverPairList> component (already established during a separate merge) over this branch's now-superseded inline <ul> rendering; extended LeftoverPairList itself with the explained-share tier so the feature isn't lost. - backend/app/report_ingestion.py, scripts/seed_demo_data.py: both insert/select statements needed the full column set (residual, observed/expected, rank, explained share) merged together. - 9 doc files (AGENTS.md, ARCHITECTURE.md, CLAUDE.md, ADR 0048/0049, CHANGELOG.md, migrate.sh): additive concatenation of amendment descriptions, matching the fixed-lower-bound migrate.sh pattern already established elsewhere. - Fixed two pre-existing (unconflicted, untouched by either side) App.test.tsx assertions that predated the LeftoverPairList component swap and checked a stale aria-label word order. Verified: tests/ (789 passed, 11 skipped), backend/tests/ (130 passed, 5 skipped -- docker-stack-gated), frontend (234 passed), tsc clean. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NpWw9SnPBaemFZmW3fdTVM
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
# Conflicts: # AGENTS.md # ARCHITECTURE.md # CHANGELOG.md # CLAUDE.md # backend/tests/test_api.py # docs/adr/0003-fast-mlsirm-report-integration.md # frontend/package.json # lineageweave/leftover_pairs.py # pyproject.toml # tests/test_leftover_pairs.py # tests/test_period_report.py # tests/test_schema.py # uv.lock
# Conflicts: # AGENTS.md # ARCHITECTURE.md # CHANGELOG.md # CLAUDE.md # backend/app/report_ingestion.py # backend/tests/test_api.py # docs/adr/0003-fast-mlsirm-report-integration.md # docs/adr/0048-persist-lsirm-leftover-pairs.md # docs/adr/0049-leftover-pair-report-ui.md # frontend/package.json # frontend/src/App.test.tsx # frontend/src/api.ts # frontend/src/components/LeftoverPairList.tsx # frontend/src/i18n.test.ts # frontend/src/i18n.ts # lineageweave/leftover_pairs.py # pyproject.toml # scripts/seed_demo_data.py # tests/test_leftover_pairs.py # tests/test_period_report.py # tests/test_schema.py # uv.lock
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
Cycle comment: leftover increment still waits for independent APPROVE + exact-head required success. Author must not self-approve. Do not squash-merge without that APPROVE. Recut leftover-map length onto protected main as independent v2.12.31 / ADR 0187 (0183–0186 claimed by open leftover share/reconstruction PRs). Do not mix leftover stacks. Issues #79 and #87 stay open. |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
The identical redefinition at the bottom of the file silently shadowed the first, so pytest collected it once but code-quality flagged the dead assignment. Review follow-up on this PR's thread. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VENX71RtEntaUq6nkAWZho
|
Addressed in the pushed commit: the identical redefinition of |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
# Conflicts: # AGENTS.md # CHANGELOG.md # lineageweave/leftover_pairs.py # tests/test_leftover_pairs.py
The branch's ADR 0184 collided with main's 0184-ontology-provenance-explorer after the main merge; the explained-share ADR and every reference to it (AGENTS, ARCHITECTURE, CHANGELOG + fragment, module and component docstrings, tests) now use 0185. Also updates the sparse-map test to the ADR 0168 contract merged on main: no complete-case rectangle names no pairs at all, so the old omit-share fallback assertion is replaced by the empty-pairs contract. hygiene+leftover+schema suites 42/42. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VENX71RtEntaUq6nkAWZho
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
The prior renumbering (ADR 0184 -> 0185) only touched the ADR file and left the migration/rollback pair, several cross-references, and one broken relative link (ADR 0049) still pointing at 0184. It also blind-replaced 0184->0185 in AGENTS.md/ARCHITECTURE.md/CHANGELOG.md without distinguishing the unrelated ontology-provenance-explorer ADR, which still legitimately owns 0184 -- those got reverted back. Migration and rollback files renamed 0184 -> 0185 to match the ADR number (matches the codebase's established one-number-per-slice convention), with every test/script path reference updated.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
Thread dispositions: the badge's compact |
# Conflicts: # CLAUDE.md
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
# Conflicts: # AGENTS.md # ARCHITECTURE.md # docs/adr/0049-leftover-pair-report-ui.md
The 7-element tuple passed only because the negative-rank guard raised before _pair_from_candidate read row[7]; the row now carries all 8 elements so the test exercises the guard, not an IndexError shield. Review follow-up on this PR's thread. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VENX71RtEntaUq6nkAWZho
|
Both threads addressed: the negative-rank test now passes a full 8-element candidate row (pushed), so it exercises the guard instead of surviving by short-circuit; and the comparison strip's reduced payload is the established contract — rank, observed/expected, and unexplained are likewise report-panel-only, with the strip carrying distance+residual and the full detail one click away on the named post. Resolving. |
|
Closed because protected main now normatively rejects this per-cell share alias. ADR 0003 defines the auditable raw-residual identity through U = R − R̂ and x = 2 R̂U/R², and explicitly says not to persist explained share e or unexplained share s. Keeping this branch open would conflict with the accepted exact-main measurement contract. |
…588) * feat: name leftover-map reconstruction on leftover pairs (v2.12.31) Persist two-axis Gabriel reconstruction R̂ = ξ_{1:2} · ζ_{1:2} on period leftover pairs so landed unexplained leftover U = R − R̂ stays auditable as U + R̂ = R (ADR 0183). Reconstruction is the same internal two-axis inner product already used for U; do not substitute the centered R̃ reconstruction claimed by leftover stack #564. After make seed, closest and farthest leftover pairs sit above the member list with signed R̂ next to leftover-map distance d; click opens that post. Missing or non-finite reconstruction omits the badge rather than inventing a leftover score. Signed reconstruction is stored, never clamped. Complete-case coverage (ADR 0168) stays independent. Independent of leftover stacks #485, #518 (landed), #521, #537, #539, #563, #564, and #579. * fix(adr): allocate reconstruction decision 0201 * fix: restore R̂-persistence docs and finish ADR 0183→0201 renumbering The prior merge of origin/main into this branch (91d8e4f) resolved the AGENTS.md/ARCHITECTURE.md/CLAUDE.md conflicts by dropping this PR's own leftover-map reconstruction documentation: - AGENTS.md reverted to "Two-axis reconstruction R̂ is not persisted", contradicting the shipped migration/ADR 0201 that persists it, and dropped ADR 0201 from the governing-ADR list. - ARCHITECTURE.md kept a redundant "0183 / 0201" pair (0183 is this repo's real, unrelated ADR 0183 "GNB four Korean chrome"; the stale 0183 leftover-map reference should have been renumbered to 0201, not kept alongside it). - CLAUDE.md's "Where the rest lives" pointer never got the ADR 0201 cross-reference added. Also finishes the ADR-number renumbering the PR's own history had started (docs/adr/0201-leftover-map-reconstruction.md is the actual ADR; ADR 0183 already belongs to a different, already-landed decision on main): fixes remaining stale "ADR 0183" citations in docs/adr/0003, the 2.12.31 CHANGELOG.d fragment, and the migration 0183 SQL header comment, all of which should cite ADR 0201. docs/adr/0049's own conflict resolution (0158 + 0201 amendments) was already correct and is unchanged. * docs: reconcile ADR 0182 reconstruction context * test(leftover): cover reconstruction candidate contract * fix(leftover): prove rank-zero reconstruction identity --------- Co-authored-by: seonghobae <seonghobae@users.noreply.github.com>
1 participant
Buyer next action
After
make seed, leftover post–criterion pairs sit above the member list with explained leftover shareR̂²/R̃²next to leftover-map distanced. Click the pair to open that post. The share badge is omitted when the pair has no complete-case leftover map (fallback rows do not invent a share). Never invent leftover score or theta.A leftover residual
Ris not the leftover the two leftover-map axes reconstruct. Explained leftover share namese = R̂_c² / R̃²of centered leftoverR̃ = R − centerafter two-axis reconstructionR̂_c = ξ_{1:2} · ζ_{1:2}. Using rawRin the denominator leaves the grand mean inside the named leftover, so a fully reconstructed rank-1 cell would look only partly explained whenevercenter ≠ 0. This PR centers first. Per-celleis not1 − s: truncated two-axis reconstruction keeps a cross term2 R̂_c U_c.R̂_candU_cstay internal and are not persisted.A 0/0 origin cell (
R̃ = 0andR̂_c = 0) storese = 0, including the origin cell of a rank-1 map. A reconstructed nonzero rank-1 cell storese = 1even whenmean(R) ≠ 0.Scope (this PR only)
leftover_map_explained_shareon leftover pair rowsleftover_map_explained_sharewhen complete-case ξ, ζ exist (e = R̂_c² / R̃²; unused axes pad with 0; hidden SVD axes after the second are dropped)R̂²/R̃²badge + named next action; i18n en/ko/zh/ja/vimean(R) ≠ 0; rank-3 synthetic residual provese ≠ R,≠ leftover_distance, ande + s ≠ 1(cross term); leftover_distance stays full-rank Euclideane = 0; fallback omits shareleftover_map_unexplained_share,leftover_map_unexplained, orleftover_map_reconstructionaccessToken(login screen has no token) and drop unusedoidcReturnUrlimport so leftover UI typechecksIndependent of leftover stacks already in review
Do not mix into:
Merge contract
b15d0ee31f6dc71f5dae2839a45d43b6716e5911APPROVErequired (Copilot or human who is not the author)Summary by CodeRabbit
새 기능
개선 사항