feat: name leftover-map cross share on leftover pairs (v2.12.29) - #563
feat: name leftover-map cross share on leftover pairs (v2.12.29)#563seonghobae wants to merge 11 commits into
Conversation
Persist leftover-map cross share x = 2 R̂_c U_c / R̃² of centered leftover on leftover post–criterion pairs (ADR 0185). After make seed, closest and farthest leftover pairs sit above the member list with 2R̂U/R̃² next to leftover-map distance d; click opens that post. A signed remainder is shown, never clamped. Do not invent a leftover score. Do not persist explained leftover share e, unexplained leftover share s, unexplained leftover U, or reconstruction R̂.
|
Warning Review limit reachedNext included review available in 21 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 (19)
📝 WalkthroughWalkthrough기간 보고서 leftover pair가 ChangesLeftover-map cross-share
AdminPanel 인증 가드
Estimated code review effort: 4 (Complex) | ~45 minutes Merge Risk: 🟡 Moderate · up to This PR adds and persists leftover-map cross-share values and exposes them in the UI, but the current implementation can omit valid shares for small finite inputs, and an unsafe rollback order can cause report queries to fail when the new column is removed before the application is downgraded. These bounded correctness and deployment risks should be fixed or explicitly accepted before merge. Sequence Diagram(s)sequenceDiagram
participant PeriodReport
participant leftover_pairs
participant report_ingestion
participant PostgreSQL
participant App
PeriodReport->>leftover_pairs: leftover pair 계산
leftover_pairs-->>PeriodReport: cross-share 포함 LeftoverPair 반환
PeriodReport->>report_ingestion: report 저장 요청
report_ingestion->>PostgreSQL: cross-share 저장
App->>PostgreSQL: leftover pair 조회
PostgreSQL-->>App: cross-share와 distance 반환
App-->>App: cross-share 포맷 및 post 링크 표시
🚥 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 |
![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.
![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.
Replace the self-comparison NaN check with math.isnan and the literal infinity comparison with math.isinf so the assertion intent is explicit (github-code-quality review thread).
![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.
# Conflicts: # AGENTS.md # ARCHITECTURE.md # CHANGELOG.md # CLAUDE.md # backend/app/report_ingestion.py # backend/tests/test_api.py # docker/postgres-init/migrate.sh # 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/App.tsx # frontend/src/api.ts # frontend/src/components/AdminPanel.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_migration_replay.py # tests/test_period_report.py # tests/test_schema.py # uv.lock
The 3-way merge of feat/leftover-map-cross-share-v21229 into main silently dropped main's rememberOidcReturnUrl/returnUrlFromLocation import (git resolved the adjacent import-block edits without a conflict marker), breaking tsc with two TS2304 errors. Restore it.
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 threshold
sqrt(1e-12) = 1e-6, so a small-but-finite centered leftover (R-tilde =
1e-7, reconstruction 5e-8, x = 0.5) collapsed to an omitted badge.
Compare absolute values first; regression test added.
Also rename the Vietnamese cross-term label from phần dư đồng nhất
(reads as 'identical residual') to phần giao ('cross part'), matching
the 2 R-hat_c U_c / R-tilde-squared quantity it names; i18n expectation
updated (coderabbit review threads).
| def _leftover_map_cross_share(filled: float, reconstruction: float) -> float | None: | ||
| """Return ``x = 2 R̂_c U_c / R̃²`` when both terms are finite; otherwise omit. | ||
|
|
||
| ``filled`` is centered leftover ``R̃ = R − center``. Unexplained | ||
| leftover ``U_c = R̃ − R̂_c`` is computed internally. Truncated | ||
| two-axis reconstruction of a higher-rank cell keeps a cross term | ||
| ``2 R̂_c U_c``, so per-cell ``e + s ≠ 1``. The identity remainder | ||
| ``x`` names that cross term as a share of centered leftover. | ||
| ``x`` may be negative when reconstruction and unexplained leftover | ||
| have opposite signs; a negative finite share is stored, not omitted. | ||
| """ | ||
| if not np.isfinite(filled) or not np.isfinite(reconstruction): | ||
| return None | ||
| unexplained = float(filled - reconstruction) | ||
| # Threshold on absolute magnitudes, not squares: squaring first makes the | ||
| # effective floor sqrt(1e-12) = 1e-6 and collapses small-but-finite cells | ||
| # (e.g. R-tilde = 1e-7 with a valid cross term) to an omitted badge. | ||
| if abs(filled) > _LEFTOVER_SINGULAR_FLOOR: | ||
| share = float(2.0 * reconstruction * unexplained / (filled * filled)) | ||
| return share if np.isfinite(share) else None | ||
| if abs(reconstruction) <= _LEFTOVER_SINGULAR_FLOOR and abs(unexplained) <= _LEFTOVER_SINGULAR_FLOOR: | ||
| return 0.0 | ||
| return None |
There was a problem hiding this comment.
📝 Info: Cross-share floor thresholds on magnitude, not square
_leftover_map_cross_share in leftover_pairs.py gates on abs(filled) > 1e-12 so a valid small centered leftover (e.g. R̃ = 1e-7) keeps its share instead of collapsing at an effective 1e-6 floor. Reconstructed rank-1 and origin cells return 0.0 because U_c = 0.
Was this helpful? React with 👍 or 👎 to provide feedback.
# 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
seed() called _seed_demo_period_report, which persists rows into report_leftover_map_axis, but never ran migration 0169 that creates that table -- inherited from the leftover-map axis-share merge and would crash `make seed` on a fresh database. The seeded_db/schema_db pytest fixtures already ran 0169 directly, so no test caught this.
The merge changed _leftover_map_positions' third return value from an int rank (PR563's own contract, needed only a count) to the raw kept singular-value array (origin/main's axis-share contract, needed for Gabriel inertia) -- both callers now use the array. This test still unpacked it as a scalar and compared it directly to an int, which raised on any residual matrix with more than one kept singular value.
# 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
| filled = float(residual[person, item]) - center | ||
| unexplained = _unexplained_leftover(float(residual[person, item]), reconstruction) | ||
| share = _leftover_map_cross_share(filled, reconstruction) |
There was a problem hiding this comment.
📝 Info: Cross share uses centered leftover, unexplained uses raw
_leftover_map_cross_share receives filled = residual − center (centered R̃), while _unexplained_leftover receives the raw residual R. The two adjacent persisted columns are therefore computed on different bases — deliberate per ADR 0185, but easy to misread when maintaining this code.
Was this helpful? React with 👍 or 👎 to provide feedback.
| if abs(reconstruction) <= _LEFTOVER_SINGULAR_FLOOR and abs(unexplained) <= _LEFTOVER_SINGULAR_FLOOR: | ||
| return 0.0 | ||
| return None |
There was a problem hiding this comment.
📝 Info: Zero-cross-share branch can miss values straddling the floor
The return 0.0 branch requires both reconstruction and unexplained at or below the 1e-12 floor. Because unexplained = filled − reconstruction, two near-floor values with opposite signs push abs(unexplained) above the floor, so the badge is omitted rather than stored as 0.0. Degenerate boundary only; omitting is a safe fallback.
Was this helpful? React with 👍 or 👎 to provide feedback.
|
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. |
Merge commit only: two test-side gaps left by clean auto-merge. _candidate_row() gained leftover_map_cross_share upstream; one direct unit-test call site (the residual-boundary rejection test) wasn't updated to pass it, so importing the test module raised a TypeError. Separately, the API test's per-pair assertions both validated leftover_map_unexplained's shape (added by an already-merged main PR) and asserted its absence (this branch's original, now-stale expectation) side by side -- drop the stale negative assertion.
| cur.execute((migrations / "0012_report_leftover_pair.sql").read_text()) | ||
| cur.execute((migrations / "0163_report_leftover_observed_expected.sql").read_text()) | ||
| cur.execute((migrations / "0164_report_leftover_map_rank.sql").read_text()) | ||
| cur.execute((migrations / "0169_report_leftover_map_axis.sql").read_text()) |
There was a problem hiding this comment.
📝 Info: Seed gains the 0169 axis migration its axis insert relies on
The seed migration list now includes 0169_report_leftover_map_axis.sql. The seed already inserts into report_leftover_map_axis via _persist_seed_period_report (seed_demo_data.py), but the base-commit list omitted 0169. The migration is CREATE TABLE IF NOT EXISTS, so re-application is harmless.
Was this helpful? React with 👍 or 👎 to provide feedback.
1 participant
Buyer next action
After
make seed, leftover post–criterion pairs sit above the member list with leftover-map cross share2R̂U/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). A signed remainder is shown, never clamped. Never invent leftover score or theta.A leftover residual
Ris not the leftover the two leftover-map axes reconstruct. Truncated two-axis reconstruction of a higher-rank cell keeps unexplained leftoverU_c = R̃ − R̂_cand a cross term2 R̂_c U_c. The cell identityR̃² = R̂_c² + U_c² + 2 R̂_c U_ctherefore yieldse + s + x = 1with explained leftover sharee = R̂_c² / R̃², unexplained leftover shares = U_c² / R̃², and leftover-map cross sharex = 2 R̂_c U_c / R̃². Hidingxlets a buyer reade + sas a complete leftover partition even though the truncated map leaves an identity remainder.xmay be negative when reconstruction and unexplained leftover have opposite signs.R̂_candU_cstay internal and are not persisted. This PR does not persisteors.A 0/0 origin cell (
R̃ = 0,R̂_c = 0,U_c = 0) storesx = 0, including the origin cell of a rank-1 map. A reconstructed nonzero rank-1 cell storesx = 0becauseU_c = 0even whenmean(R) ≠ 0.Scope (this PR only)
leftover_map_cross_shareon leftover pair rows (no nonnegative CHECK)leftover_map_cross_sharewhen complete-case ξ, ζ exist (x = 2 R̂_c U_c / R̃²; unused axes pad with 0; hidden SVD axes after the second are dropped)2R̂U/R̃²badge + named identity-remainder next action; i18n en/ko/zh/ja/vimean(R) ≠ 0; rank-3 synthetic residual provesx ≠ R,≠ leftover_distance, and internallye + s + x = 1without persistinge/s/R̂/U; leftover_distance stays full-rank Euclideanx = 0; fallback omits share; negative finite share is storedleftover_map_explained_share,leftover_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
APPROVErequired (Copilot or human who is not the author)Summary by CodeRabbit
새 기능
버그 수정
문서