Skip to content

Thread styling kwargs through plotly visualization backends#244

Merged
igerber merged 1 commit intomainfrom
plotly-kwargs
Mar 29, 2026
Merged

Thread styling kwargs through plotly visualization backends#244
igerber merged 1 commit intomainfrom
plotly-kwargs

Conversation

@igerber
Copy link
Copy Markdown
Owner

@igerber igerber commented Mar 29, 2026

Summary

  • Thread marker, markersize, linewidth, capsize, ci_linewidth, and show_grid kwargs from public plot functions through to their plotly renderers
  • Add _mpl_marker_to_plotly_symbol() helper in _common.py for matplotlib-to-plotly marker symbol conversion
  • Convert plot_bacon scatter markersize (area in points²) to plotly diameter (px) via sqrt conversion
  • linewidth/capsize not passed to event study plotly renderers (plotly uses CI bands, not error bars)
  • 13 new tests verifying kwargs reach plotly trace properties
  • Remove resolved TODO item

Methodology references

  • N/A — no methodology changes, visualization only

Validation

  • Tests added/updated: tests/test_visualization_plotly.py (13 new tests)
  • All 114 visualization tests pass (33 plotly + 81 matplotlib)
  • Formatting (black) and linting (ruff) clean

Security / privacy

  • Confirm no secrets/PII in this PR: Yes

Generated with Claude Code

Plotly renderers previously ignored marker, markersize, linewidth,
capsize, ci_linewidth, and show_grid kwargs that the matplotlib
backend honored. This threads them through to the plotly renderers
with appropriate mappings (marker symbol conversion, scatter area-to-
diameter conversion for plot_bacon).

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@github-actions
Copy link
Copy Markdown

Overall Assessment

✅ Looks good

Executive Summary

  • Cross-checking the touched areas against the Methodology Registry found no estimator, weighting, identification, variance, or default-behavior change. This PR is limited to visualization plumbing for Bacon, HonestDiD sensitivity/event-study, PowerAnalysis, and PreTrendsPower.
  • The main plotly threading work is correct for ci_linewidth, most marker/markersize paths, and linewidth/show_grid on the power plots.
  • plot_event_study() and plot_honest_event_study() still leave some public styling kwargs silently ignored on plotly, so the old styling-gap TODO is not fully resolved.
  • The new marker helper silently coerces unsupported matplotlib markers to "circle" and uses the same hollow-marker recipe for line-only symbols like "+"/"x", which does not preserve matplotlib semantics on plotly.
  • New tests cover happy-path supported kwargs, but not the remaining ignored/rejected-kwargs paths or line-only reference-marker behavior.
  • Plotly is not installed in this review environment, so plotly-specific rendering comments are from static review rather than execution.

Methodology

Code Quality

Performance

Maintainability

  • Severity: P3-informational. Impact: Centralizing marker translation in diff_diff/visualization/_common.py:L286-L319 is a maintainability improvement. No separate maintainability defect stood out beyond the API-contract gaps above. Concrete fix: None.

Tech Debt

  • Severity: P2. Impact: The PR removes the tracked TODO for plotly styling kwargs, but the event-study/honest-event-study gaps above remain. After merge, that limitation would no longer be tracked in the current TODO section at TODO.md:L70-L77. Concrete fix: Keep the TODO until all public styling kwargs are either supported or explicitly rejected on plotly, or replace it with a narrower TODO covering the remaining event-study limitations.

Security

  • Severity: P3-informational. Impact: No new file/network/process behavior, no secrets handling, and no obvious injection surface in the touched files. Concrete fix: None.

Documentation/Tests

  • Severity: P3. Impact: The new tests in tests/test_visualization_plotly.py:L363-L522 cover happy-path supported kwargs, but they do not cover unsupported markers, line-only reference markers, or explicit support/rejection of non-default event-study linewidth/capsize. Those are the remaining contract edges in the new code. Concrete fix: Add plotly tests for marker="+" and marker="x" with a reference period, for an unsupported matplotlib marker (expecting error/warning), and for non-default event-study linewidth/capsize on plotly (expecting either support or explicit rejection).

@igerber igerber merged commit 93abaea into main Mar 29, 2026
15 checks passed
@igerber igerber deleted the plotly-kwargs branch March 29, 2026 15:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant