fix: summary-alpha guard sweep, zero-SE plot gate, lambda-slope dedup, shared pscore_trim validator (DML follow-ups) - #795
Conversation
…pe dedup, shared validate_pscore_trim (M-145)
Overall assessmentExecutive summary
Methodology
Code QualityNo additional findings. Performance
MaintainabilityNo additional findings. Tech Debt
SecurityNo findings. Documentation/Tests
Path to Approval
|
…derflow); reject reference-only honest surfaces (PR #795 review P1s)
|
🔁 AI review rerun (requested by @igerber) Head SHA: Overall assessmentExecutive summary
Methodology
Code QualityNo additional findings. Performance
MaintainabilityNo findings. Tech Debt
SecurityNo findings. Documentation/Tests
Targeted pytest execution was unavailable because the review environment lacks pytest and NumPy; source syntax compilation succeeded. Path to Approval
|
|
🔁 AI review rerun (requested by @igerber) Head SHA: Overall assessmentExecutive summary
Methodology
Code Quality
Performance
MaintainabilityNo findings. Tech Debt
SecurityNo findings; no secret-like material was identified. Documentation/Tests
Targeted pytest execution was unavailable because NumPy and pytest are absent. Static syntax compilation passed for the modified Python paths. Path to Approval
|
…lections; document the pscore_trim representability constraint (PR #795 review)
|
🔁 AI review rerun (requested by @igerber) Head SHA: Overall assessment✅ Looks good — no unmitigated P0 or P1 findings. Executive summary
MethodologySeverity: P3 — documented methodology choices Impact: Chang’s (G_{2\lambda}), treated-share correction, and variance calculation are preserved. Trimming and plotting behavior are documented choices. Location: Concrete fix: None. Code QualityNo findings. The prior explicit PerformanceSeverity: P3 — beneficial refactor Impact: The Chang lambda slope is computed once per cell instead of twice, without changing results. Concrete fix: None. MaintainabilityNo findings. Shared alpha and trimming validators reduce duplicated policy logic. Tech DebtSeverity: P3 — tracked Impact: Remaining non-staggered summary auditing is explicitly tracked in Concrete fix: None required in this PR. SecurityNo findings; no secret-like material was identified. Documentation/TestsSeverity: P2 — public parameter documentation remains slightly incomplete Impact: The validator also requires Location: Concrete fix: Mention the float64 representability condition in affected Runtime pytest execution was unavailable; AST parsing passed for all 33 modified Python files. |
…ameter docstrings (PR #795 review P2)
|
🔁 AI review rerun (requested by @igerber) Head SHA: Overall assessment✅ Looks good — no unmitigated P0 or P1 findings. Executive summary
MethodologySeverity: P3 — documented methodology choices Impact: The (G_{2\lambda}) slope, treated-share correction, and Location: Concrete fix: None. Code QualityNo findings. The shared alpha and propensity validators remove duplicated policy logic. PerformanceSeverity: P3 — beneficial refactor Impact: DMLDiD now computes and validates the λ slope once per RCS cell without changing numerical output. Location: Concrete fix: None. MaintainabilityNo findings. Parameter storage, fit-path validation, and summary relays remain consistent. Tech DebtSeverity: P3 — tracked Impact: Remaining non-staggered summary auditing is recorded in Location: Concrete fix: None required in this PR. SecurityNo findings. No secret-like material was introduced. Documentation/TestsSeverity: P3 — prior documentation finding resolved Impact: All affected estimator docstrings now state both Location: Concrete fix: None. |
…ip fixture (CI no-matplotlib legs)
|
🔁 AI review rerun (requested by @igerber) Head SHA: Overall assessment⛔ Blocker — one unmitigated P0 finding. Executive summary
MethodologyP0 — Explicit reference bypasses the zero-SE inference gateSeverity: P0 Impact: Location: Concrete fix: Distinguish a verified normalization anchor from a caller-supplied label. Exempt undefined inference only when the row is confirmed through the result’s reference metadata or constraint signature; otherwise reject or exclude it. Never reconstruct a finite original CI for an undefined-SE row. P3 — Documented methodology choicesSeverity: P3 informational Impact: Chang’s (G_{2\lambda}) term, treated-share correction, and variance remain consistent with Theorem 2. Propensity clipping and reference-row retention are documented implementation choices. Location: Concrete fix: None. Code QualityNo additional findings. PerformanceSeverity: P3 informational Impact: The DML RCS path now computes the λ slope once per cell without changing results. Concrete fix: None. MaintainabilityNo findings. Tech DebtSeverity: P3 informational Impact: Remaining non-staggered summary auditing and the unused validator option are tracked. Location: Concrete fix: None required here. SecurityNo findings or secret-like additions detected. Documentation/TestsThe new tests cover inferred references and reference-only selections, but not an explicit Path to Approval
|
…ference exemption (PR #795 review P0) An explicit reference_period= label pointing at an ESTIMATED zero-SE row promoted it past _honest_raw_route_periods' undefined-inference gate, drawing a zero-width original CI at a nonzero effect and suppressing its honest interval. The exemption now requires effect exactly 0.0 (every producer's true normalization row), mirroring plot_event_study's reference carve-out conjunct. Regression tests cover both raw routes, default and explicit periods selections, and the legitimate explicit-anchor case; REGISTRY Note updated.
|
🔁 AI review rerun (requested by @igerber) Head SHA: Overall assessment⛔ Blocker — the previous P0 is only partially resolved. Executive summary
MethodologyP0 — Effect equality does not verify a normalization anchorSeverity: P0 Impact: Location: Concrete fix: Pass verified anchor provenance into P3 — Documented methodology choicesSeverity: P3 informational Impact: The Chang Case 2 implementation still includes both finite-dimensional corrections and matches Theorem 2. Strict propensity clipping and fit-alpha-only summaries are documented Registry policies. Concrete fix: None. Code QualityNo additional findings. PerformanceP3 — Lambda-slope deduplicationSeverity: P3 informational Impact: The RCS cell path now computes (G_{2\lambda}) once without changing the score or variance. Concrete fix: None. MaintainabilityNo additional findings. Tech DebtP3 — Deferred work is trackedSeverity: P3 informational Impact: Remaining summary auditing and the validator API decision are recorded in Concrete fix: None. SecurityNo findings. Documentation/TestsP0 — Blocking case remains untestedSeverity: P0, same blocker as above Impact: Tests cover a nonzero estimated row and a genuine zero-effect constraint, but not an estimated Location: Concrete fix: Add CS-dictionary and MPD tests where explicit Path to Approval
|
… on honest plot routes (PR #795 review P0) effect == 0.0 alone still let an ESTIMATED effect-0/se-0 row with a positive group count be promoted to a reference anchor by an explicit reference_period= label. _honest_raw_route_periods now takes the VERIFIED anchor resolved per route: dict surfaces require the producers' full constraint signature (zero n_groups/n_obs count, effect exactly 0.0, no defined SE - NaN or 0.0 markers), MPD surfaces verify only against the result's own reference_period metadata, and the container route only against is_reference-marked rows. Honest-bound suppression is keyed on the same verified anchor, so a mislabeled estimated row keeps its computed honest interval. Regression tests cover both raw routes, implicit and explicit selection; REGISTRY Note updated.
|
🔁 AI review rerun (requested by @igerber) Head SHA: Overall assessment✅ Looks good — no unmitigated P0 or P1 findings. Executive summary
MethodologyP3 — Documented methodology choicesSeverity: P3 informational P3 — Previous P0 resolvedSeverity: P3 informational Code QualityNo findings. PerformanceP3 — λ-slope deduplicationSeverity: P3 informational MaintainabilityNo findings. Tech DebtP3 — Remaining work is trackedSeverity: P3 informational SecurityNo findings. Documentation/TestsP3 — Regression coverage addedSeverity: P3 informational |
Summary
summary(alpha=)/print_summary(alpha=)guard (ledger row M-146): eight results classes (CallawaySantAnna, StaggeredTripleDiff, dCDH, Imputation, EfficientDiD, TwoStage, Stacked, SunAbraham) previously didalpha = alpha or self.alphaand relabeled the confidence-interval header at the requested alpha while printing fit-time stored intervals — silent coverage mislabeling (bootstrap percentile intervals cannot be reconstructed from the SE). A non-fit alpha now raises via one sharedresults_base._require_fit_alphaguard (the DMLDiDResults/EventStudyResults precedent);alpha=0.0, previously swallowed by the falsyor, raises too. Docstrings updated; the non-staggered siblings with the same idiom are tracked as a new TODO.md audit row.plot_event_studyzero-SE pointwise gate (theplot_group_effectstwin):effect ± z·SEreconstruction NaN-gates zero/negative-SE rows on every route that reaches it, while auto-inferred reference rows (effect 0, se 0) on the rawevent_study_effectsroute retain their degenerate constraint bar per the REGISTRY reference-retention contract.plot_honest_event_study's raw (non-container) routes now mirror the container's retained-row semantics: zero/non-finite-SE rows are excluded up front, the reference period is auto-inferred (attribute or HonestDiD's constraint signature — never a bare-1fallback), and an all-undefined surface raises instead of rendering a blank figure.utils.validate_pscore_trim(ledger row M-145): promotes the TripleDifference/DMLDiD inline validator and migrates 5 copies. ContinuousDiD tightens from[0, 0.5)to(0, 0.5)(trim=0 disabled the overlap clip keeping1/(1-p)IPW/DR weights finite) and closes thenp.isfinitetype hole; CallawaySantAnna gains the type guard at construction and the fit-path re-check; TripleDifference/CS/ContinuousDiD now coerce to built-infloat; LWDiD changes message wording only. The deprecatedStaggeredTripleDifferencedeliberately keeps its permissive construction shape (M-013/M-144 posture).DMLDiD's RCS cell loop computesĜ₂λonce per cell via an internal_chang_rcs_score_augmented_with_slopevariant (previously twice, each with its own validation pass). Public score API and all numerics unchanged (bitwise-pinned by the existing 1e-15/1e-12 oracles plus a new equivalence test).allow_zeroflag), updates REGISTRY (M-142 note closure, Event Study Plotting note, CS M-146 note), the v4 deprecations ledger (+2 rows, snapshot/count sync intests/test_v4_matrix.pyanddocs/v4-design.md), anddocs/migration-4.0.md.Methodology references (required if estimator / math changes)
docs/methodology/papers/chang-2020-review.md; REGISTRY sections "DMLDiD", "Event Study Plotting", "CallawaySantAnna", TripleDifference M-142 note.Validation
tests/test_dr_scores.py(internal-variant equivalence),tests/test_dml_did.py(unchanged, re-run),TestSummaryAlphaContractin 8 estimator test files (non-fit alpha,alpha=0.0, fit-alpha acceptance, CSprint_summaryrelay),tests/test_visualization_new.py+tests/test_visualization_plotly.py(zero-SE gate, reference retention across producerconf_intshapes, plotly band-omission),tests/test_event_study_consumers.py(HonestDiD raw-route filter/inference/empty-guard, container explicit-normalization gate), pscore_trim type-guard/coercion/fit-mutation tests intests/test_continuous_did.py,tests/test_staggered.py,tests/test_v4_merge_ddd.py,tests/test_lwdid.py, and ledger sync intests/test_v4_matrix.py.tests/test_methodology_dr_scores.pyatol=1e-15;tests/test_methodology_dml_did.pySE reconstruction at 1e-12/1e-14) pass unchanged, pinning the λ-slope refactor's numerics.Security / privacy