Skip to content

Fix plot_event_study reference_period normalization #108

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
igerber merged 4 commits into from
Jan 25, 2026
Merged

Fix plot_event_study reference_period normalization #108

igerber merged 4 commits into from
Jan 25, 2026

Conversation

@igerber
Copy link
Owner

@igerber igerber commented Jan 24, 2026

Summary

  • Fix plot_event_study to actually normalize effects when reference_period is specified
  • The parameter was documented as normalizing effects to 0 but only applied visual styling (hollow marker)
  • Now subtracts the reference effect from all effects so reference period appears at y=0
  • Add 3 tests verifying normalization behavior
  • Fix Python 3.9 compatibility issue in tutorial 02 (backslash in f-string)

Methodology references (required if estimator / math changes)

  • Method name(s): N/A - visualization fix only, no statistical methodology changes
  • Paper / source link(s): N/A
  • Any intentional deviations from the source (and why): None

Validation

  • Tests added/updated: tests/test_visualization.py (3 new tests)
    • test_plot_event_study_reference_period_normalization - verifies normalization occurs
    • test_plot_event_study_no_normalization_without_reference - verifies no change when reference_period=None
    • test_plot_event_study_normalization_with_nan_reference - verifies NaN reference is handled
  • Backtest / simulation / notebook evidence (if applicable): Tutorial 02_staggered_did.ipynb validated via /validate-tutorial 02

Security / privacy

  • Confirm no secrets/PII in this PR: Yes

Generated with Claude Code

@igerber @claude
Fix plot_event_study reference_period normalization
5513acf 
The reference_period parameter was documented as normalizing effects
to 0 but only applied visual styling. This commit adds actual
normalization logic that subtracts the reference effect from all
effects when reference_period is specified.

Changes:
- Add normalization logic in plot_event_study() after critical_value
  calculation: subtracts reference effect from all effects
- Only normalizes if reference_period exists in data and is finite
- Add 3 tests verifying normalization behavior, no-normalization case,
  and NaN reference handling
- Fix Python 3.9 compatibility issue in tutorial notebook (backslash
  in f-string expression)

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
@github-actions
Copy link

github-actions bot commented Jan 24, 2026

Overall assessment: ⚠️ Needs changes

Executive summary

  • P1: plot_event_study now re-centers effects but keeps original SEs, so CIs can be wrong when the reference effect is estimated (diff_diff/visualization.py:L195-L214).
  • Affects event-study plots for MultiPeriodDiD / CallawaySantAnna / SunAbraham; the registry expects normalization at estimation, so post‑hoc shifts should not alter inference without variance adjustment.
  • Tests added cover point‑estimate normalization and NaN reference handling, but CI behavior on the new normalization path isn’t exercised.

Methodology

  • P1 | Impact: When reference_period is in effects, the function subtracts ref_effect yet computes CIs from unadjusted se; if inputs are not already normalized, uncertainty is understated and pre‑trends inference can be misleading. This diverges from the registry framing where normalization is a model constraint, not a post‑hoc subtraction. | Fix: only normalize when the reference effect is a known normalization constraint (e.g., effect==0 with n_groups==0), or accept a covariance matrix and compute SEs for differences; otherwise warn and drop CIs (set to NaN) when applying post‑hoc normalization. (diff_diff/visualization.py:L195-L214)

Code Quality

  • No findings.

Performance

  • No findings.

Maintainability

  • No findings.

Tech Debt

  • No findings.

Security

  • No findings.

Documentation/Tests

  • P3 | Impact: New tests verify point‑estimate re‑centering but do not validate CI/SE behavior on the normalization path, so a variance fix could regress silently. | Fix: add a test that asserts CI behavior when normalization is applied (or that CIs are suppressed when covariance is unavailable). (tests/test_visualization.py:L321-L419)

@igerber @claude
Set reference period SE to NaN during normalization
bbe7637 
When normalizing effects to a reference period, the reference SE is now
set to NaN since it's an identifying constraint rather than an estimated
quantity. This follows the fixest (R) convention where the omitted
category has no associated uncertainty.

Changes:
- visualization.py: Set reference SE to NaN when normalizing
- test_visualization.py: Verify reference period has no error bars
- REGISTRY.md: Document event study plotting normalization behavior

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
@igerber
Copy link
Owner Author

igerber commented Jan 25, 2026

/ai-review

@github-actions
Copy link

github-actions bot commented Jan 25, 2026

🔁 AI review rerun (requested by @igerber)

Head SHA: bbe7637f0f3b1f6f07acf6e0f40b3177e496cfc0

Overall Assessment

  • ⚠️ Needs changes

Executive Summary

  • Auto-normalization now triggers for inferred reference_period, re-centering CallawaySantAnna base_period="varying" plots despite no fixed baseline (P1).
  • This is a default behavior change and conflicts with the Methodology Registry’s base-period semantics, risking misinterpretation of pre-trend dynamics.
  • Tests cover only the DataFrame path; results-object paths and base_period interactions are untested (P1).
  • New tests rely on Matplotlib internals (get_children/get_segments) and may be version-fragile (P3).
  • Tests not run here; suggest pytest tests/test_visualization.py -k plot_event_study.

Methodology

  • Severity: P1; Impact: plot_event_study normalizes whenever reference_period is non-None, but _extract_plot_data falls back to -1 when no normalization constraint exists (e.g., CallawaySantAnna base_period="varying"), re-centering effects and implying an omitted-category baseline that the estimator and registry do not define; Fix: only normalize when the reference is caller-specified or explicitly flagged as a constraint (n_groups==0), otherwise keep effects unchanged and use reference_period only for styling; document whichever behavior you keep (diff_diff/visualization.py#L195, diff_diff/visualization.py#L374, docs/methodology/REGISTRY.md#L187).

Code Quality

  • No findings.

Performance

  • No findings.

Maintainability

  • No findings.

Tech Debt

  • No findings.

Security

  • No findings.

Documentation/Tests

  • Severity: P1; Impact: New normalization behavior is only tested for DataFrame input; results-based paths (CallawaySantAnna with base_period="varying"/"universal" and SunAbraham) plus the fallback reference inference are untested, so the default re-centering regression could slip; Fix: add tests that exercise plot_event_study(results) with each estimator and assert normalization only when a true reference constraint exists or when reference_period is explicitly passed (tests/test_visualization.py#L321).
  • Severity: P3; Impact: Tests scrape Matplotlib internals via ax.get_children() and get_segments(), which can be brittle across versions; Fix: assert against stable artist collections (ax.lines/ax.collections) or return explicit handles from plot_event_study for testing (tests/test_visualization.py#L344).

@igerber @claude
Only normalize when reference_period is explicitly specified
60c04cb 
- Add reference_inferred flag to _extract_plot_data return value
- Track whether reference_period was user-provided vs auto-detected
- Auto-inferred reference periods get hollow marker styling only, no normalization
- Explicit reference_period=X triggers normalization and sets ref SE to NaN
- Add tests for both auto-inferred (no normalization) and explicit (normalizes)
- Update REGISTRY.md to document the explicit vs inferred behavior

This prevents unintended normalization when reference period isn't a true
identifying constraint (e.g., CallawaySantAnna with base_period="varying").

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
@igerber
Copy link
Owner Author

igerber commented Jan 25, 2026

/ai-review

@github-actions
Copy link

github-actions bot commented Jan 25, 2026

🔁 AI review rerun (requested by @igerber)

Head SHA: 60c04cb2bda01c6b3f8711456b278bf6d97798ad

Overall Assessment

  • ⚠️ Needs changes

Executive Summary

  • plot_event_study docs still imply automatic normalization, but inferred references are now marker-only; update API docs to avoid misleading plots.
  • Registry update matches the explicit-only normalization behavior; no estimator math changes beyond visualization.
  • Tests cover explicit/implicit/NaN reference cases; two new CS tests re-fit the model and could be deduplicated to reduce runtime.

Methodology

  • No findings.

Code Quality

  • No findings.

Performance

  • P3 — Impact: Two new tests re-fit Callaway-Sant’Anna results, adding avoidable runtime to the suite. — Fix: Reuse the existing cs_results fixture (or add a module-scope fixture) so CallawaySantAnna.fit runs once. (tests/test_visualization.py:L469, tests/test_visualization.py:L525)

Maintainability

  • No findings.

Tech Debt

  • No findings.

Security

  • No findings.

Documentation/Tests

  • P2 — Impact: The reference_period docstring and Notes still imply reference periods are always normalized to 0, but normalization now only happens when explicitly provided; inferred references are marker-only. — Fix: Update the parameter docs and Notes to distinguish explicit vs inferred normalization. (diff_diff/visualization.py:L75, diff_diff/visualization.py:L154)

Open Questions

  • Assumption: Explicit normalization is intended to treat the reference as a fixed constraint with unchanged SEs (per registry); confirm whether this should be called out in the public docstring.

@igerber @claude
Update docs and reuse fixture per PR review feedback
710f2c1 
- Clarify reference_period docstring: explicit triggers normalization,
  auto-inferred only applies hollow marker styling
- Update Notes section to distinguish explicit vs inferred behavior
- Refactor two tests to reuse cs_results fixture instead of re-fitting

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
@igerber
Copy link
Owner Author

igerber commented Jan 25, 2026

/ai-review

@github-actions
Copy link

github-actions bot commented Jan 25, 2026

🔁 AI review rerun (requested by @igerber)

Head SHA: 710f2c1219f2312253d4edf9a8780bbe20acc51a

Overall Assessment

  • ✅ Looks good

Executive Summary

  • No P0/P1 findings; explicit-only normalization and SE handling match the Methodology Registry update (diff_diff/visualization.py#L206, docs/methodology/REGISTRY.md#L731).
  • Tests cover explicit normalization, no-normalization when reference is inferred/None, and NaN reference handling (tests/test_visualization.py#L321).
  • P3 maintainability note: plotting tests inspect Matplotlib internals and may be brittle across Matplotlib versions.

Methodology

  • No issues found; behavior aligns with the registry guidance for explicit-only normalization (diff_diff/visualization.py#L206, docs/methodology/REGISTRY.md#L731).

Code Quality

  • No issues found.

Performance

  • No issues found.

Maintainability

  • P3 | Impact: plotting tests rely on ax.get_children()/get_segments() which are backend/version sensitive and can be flaky (tests/test_visualization.py#L344). | Fix: assert on Axes.lines/Axes.collections directly, or refactor plot_event_study to expose normalized data for unit tests and keep plotting assertions minimal.

Tech Debt

  • No issues found.

Security

  • No issues found.

Documentation/Tests

  • No issues found; registry updated and tests added.

@igerber igerber merged commit e4d0044 into main Jan 25, 2026
4 checks passed
@igerber igerber deleted the fix/plot-event-study-reference-period-normalization branch January 25, 2026 11:45
@igerber igerber mentioned this pull request Jan 25, 2026
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@igerber