feat: complete Panel Tier-1 Stage C covariance - #126
Conversation
|
Plan review/fix checkpoint on exact head Reviewed Plan findings fixed across the review loop:
Fresh plan audit result: CRITICAL 0 / HIGH 0 / relevant MEDIUM 0. Plan is implementation-ready; implementation now proceeds under the same active gates. PR remains Draft. |
TheHiddenObserver
left a comment
There was a problem hiding this comment.
Fresh exact-final .claude/skills/code-review.md review on e16df4c966c3230edcf921cf18d23f583cb05aa1.
Active gates reviewed: correctness/inference, NumPy-CuPy-Torch backend behavior, public API, pinned external definitions, physical GPU provenance, performance protocol, benchmark canonicalization/catalog/coverage, generated artifacts, docs/changelogs, and regression/packaging CI. Loss/penalty/solver/CV gates are not active for this Stage-C scope except where shared hosted regression suites exercise them.
Findings after the final review/fix loop:
- CRITICAL: 0
- HIGH: 0
- relevant MEDIUM: 0
- unresolved review threads: 0
Physical evidence:
- Exact clean measurement SHA:
9c0b3050dd143c43a06bb6393d69f4f83e861637. - Artifact repository commit:
85d710bddf633134624501a9e27f03c30bc04ead. - Tesla P100 correctness: 26 estimator covariance cases + 2 direct public covariance primitives per backend, 28/28 CuPy and 28/28 Torch, requested/executed backend identity, no CPU fallback.
- Correctness canonical source:
panel-stage-c-validation-pr126-20260810-a0d258f6d6b8, 56 validation-only rows, raw SHA-256a0d258f6d6b8243e82684a29305606e5f6bd91bbe271c3ed335b32b5ec973665. - Performance canonical source:
panel-stage-c-performance-pr126-20260810-214284f02a5e, 58 synchronized timing rows, raw SHA-256214284f02a5e21e775e58deaf2fa3cc9b6384d392b96c6f300f31f4a02953b1; includes 4 explicitN=10,000, k=2, T=200QS all-lag rows and makes no speedup claim.
Post-evidence review fixes were completed before this review:
- performance canonicalization now uses T-specific scale identities (
..._t20vs..._t200) so dashboard scale filtering cannot collapse the representative high-T scenario; - the performance parser fail-closes on the exact 54-row base Cartesian matrix and exact 4-row high-T backend/case matrix;
- all manifest/source-count contracts were updated from 11 to 13 sources;
- temporary promotion/review workflows and helpers are absent from the final tree.
Physical evidence applicability audit: comparing measurement 9c0b3050... to final candidate e16df4c... shows only evidence, parser/manifest/catalog/coverage, tests, generated assets, changelogs/review record, and permanent benchmark-frontend routing changes. There is no change to statgpu/panel/**, dev/benchmarks/validate_panel_stage_c_gpu.py, or dev/benchmarks/benchmark_panel_stage_c_covariance.py after measurement, so the accepted P100 evidence remains applicable under RELEASING.md.
Generated artifact audit: frontend/docs copies are byte-identical by Git blob for benchmark data, parse report, and source inventory. Stage-C raw sources are immutable SHA-256-protected required canonical sources with explicit measurement/artifact provenance.
Exact-final hosted CI on e16df4c966c3230edcf921cf18d23f583cb05aa1: SUCCESS for Tests, Panel Stage C external covariance, Panel Stage C Torch CPU, Maintenance compatibility, Release notes validation, Release package validation, and Benchmark Frontend CI. Benchmark Frontend includes Python 3.9/3.11 parser+strict-source validation, deterministic staleness, typecheck/build, Chromium E2E, and Chromium/Firefox/WebKit production QA.
Hard exit: COMPLETE. This review considers PR #126 ready to leave Draft. This review does not merge the PR; merge remains a separate explicit action.
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e16df4c966
ℹ️ 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".
TheHiddenObserver
left a comment
There was a problem hiding this comment.
Fresh review-fix checkpoint on exact clean head c151550ab17bd9533a51599f86b6a4ea12a292e9 after the Ready-triggered Codex review.
Addressed review findings:
- P1 ordered-categorical Driscoll-Kraay chronology: ordered pandas categorical time labels preserve declared category order before NumPy coercion; unused categories are omitted so the existing observed-period contract is unchanged. Formula-side alignment preserves ordered categorical metadata through Patsy row filtering. Regression tests cover
t1,t2,t10chronology against explicit numeric time codes, missing categorical codes, and formula-aligned PooledOLS DK parity. - P2 stale CUDA/model-doc status: EN/CN model docs now state the lifecycle truth after the P1 fix. The earlier
9c0b3050...Tesla P100 artifacts passed and remain historical evidence, but the production covariance/formula change invalidates them as exact-head acceptance underRELEASING.md; PR #126 is therefore Draft pending a fresh P100 rerun.
Fresh changed+adjacent audit result on this head:
- CRITICAL: 0
- HIGH: 0
- relevant MEDIUM: 0
- unresolved review threads: 0
- temporary review workflows/helpers: 0
Exact-head permanent hosted gates: SUCCESS for Tests, Panel Stage C external covariance, Panel Stage C Torch CPU, Maintenance compatibility, Release notes validation, Release package validation, and Benchmark Frontend CI, including Python 3.9/3.11 strict-source validation, deterministic staleness, frontend build/typecheck, Chromium E2E, and Chromium/Firefox/WebKit production QA.
Hard exit: PARTIAL_REMOTE_PENDING. The only remaining gate is fresh exact-clean-head physical CUDA correctness/performance evidence for the post-review production fix. Keep PR Draft until new P100 artifacts are audited/promoted and a post-evidence fresh review is clean.
Scope
Tracks #93 and implements Panel Tier-1 Stage C / covariance completion on top of merged Stage B (#122).
Reviewed plan:
dev/plans/panel_p1_stage_c_covariance_plan.mdCurrent acceptance status — PARTIAL_REMOTE_PENDING
Current clean candidate head:
c151550ab17bd9533a51599f86b6a4ea12a292e9PR #126 is intentionally back in Draft after the Ready-triggered Codex review found two actionable issues on
e16df4c....Ready-triggered review fixes
P1 — ordered categorical Driscoll-Kraay chronology
t1,t2,t10chronology, missing categorical codes, and formula-aligned PooledOLS DK parity against explicit numeric chronology.P2 — stale physical-CUDA model documentation
9c0b3050...P100 artifacts passed, but correctly classify them as historical after the P1 production covariance fix and require a fresh exact-head rerun before Ready.Temporary review workflows/helpers have been removed from the candidate tree.
Historical physical evidence
The previous exact-clean measurement SHA was:
9c0b3050dd143c43a06bb6393d69f4f83e861637Artifact repository commit:
85d710bddf633134624501a9e27f03c30bc04eadThose Tesla P100 artifacts passed 26/26 estimator covariance cases + 2/2 direct public covariance primitives on each of CuPy and Torch and included synchronized performance evidence with the
N=10,000,k=2,T=200QS scenario. They remain valuable historical evidence, but the ordered-categorical production fix modifiesstatgpu/panel/_covariance.pyandstatgpu/panel/_formula.py; underRELEASING.md, they no longer close the new exact-head physical gate.Current lifecycle gates
Focused review-fix validation is green for Stage-C covariance, formula/API, pinned external definitions, and estimator-level external alignment. Permanent exact-head hosted workflows are being rerun on
c151550a....Remaining hard gate after hosted/fresh review convergence:
dev/benchmarks/validate_panel_stage_c_gpu.pyanddev/benchmarks/benchmark_panel_stage_c_covariance.pyon the frozen new exact clean head for both CuPy and Torch;Merge is intentionally not performed by this lifecycle; merge remains a separate explicit action.