docs: regenerate benchmark charts with all 7 set implementations #none - #45
Conversation
The committed bench_stats.csv and charts predated SortedSet and BitSet, and bench_plot.py silently dropped any implementation missing from its hardcoded IMPL_ORDER list, so the README graphs only showed 5 of the 7 set types. - add benchmarks/regenerate.sh: one-step CSV + chart regeneration (with --plot-only mode and a uv fallback when matplotlib isn't installed) - bench_plot.py: cover all 7 implementations, warn-and-plot unknown ones instead of dropping them, switch to a colorblind-safe palette with per-series markers, and use a log y-axis so the fast-path implementations are visible alongside the element-wise ones - regenerate bench_stats.csv and all 11 charts with all implementations Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VjUKQx2QYDuetR8z8GkGyU
There was a problem hiding this comment.
Pull request overview
Updates the benchmark documentation and artifacts so the README charts reflect all 7 set implementations (including SortedSet and BitSet) and can be regenerated reliably going forward.
Changes:
- Add
benchmarks/regenerate.shto regeneratebench_stats.csvand re-render charts in one step (with a--plot-onlymode). - Update
benchmarks/bench_plot.pyto include all implementations, avoid silently dropping unknown series, add markers + a colorblind-safe palette, and switch the y-axis to log scale. - Regenerate
benchmarks/bench_stats.csvwith fresh benchmark results including all implementations.
Reviewed changes
Copilot reviewed 3 out of 15 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| README.MD | Adds a quick “regenerate benchmarks” section pointing to the new script. |
| benchmarks/regenerate.sh | New helper script to (re)generate the benchmark CSV and charts. |
| benchmarks/bench_stats.csv | Updated benchmark dataset used to produce the README charts. |
| benchmarks/bench_plot.py | Updates plotting to handle all implementations and improve chart readability. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- bench_plot.py: a min of 0.00 (timer-resolution artifact rounded by the CSV's 2-decimal format) is unrepresentable on the log y-axis and was masked by matplotlib, distorting the min-max band; collapse the band to the average there instead - README: note the uv fallback so readers don't assume matplotlib must be installed globally Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VjUKQx2QYDuetR8z8GkGyU
There was a problem hiding this comment.
🟡 Not ready to approve
The new regeneration script should fail fast on unknown CLI args, and the plotting fallback for “extra” implementations should keep multiple unknown series visually distinguishable.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
Suppressed comments (2)
benchmarks/bench_plot.py:113
- For implementations present in the CSV but missing from IMPL_ORDER, everything currently defaults to black with the same marker, so multiple “extra” series become visually indistinguishable even though the header comment says identity should not rely on color alone. Assign a distinct marker/linestyle for each extra impl to keep the chart readable when more than one unknown impl appears.
color = COLORS.get(impl, '#000000')
marker = MARKERS.get(impl, 'o')
ax.plot(sizes, avgs, label=impl, color=color, linewidth=2,
marker=marker, markersize=5)
ax.fill_between(sizes, mins, maxs, alpha=0.12, color=color)
benchmarks/regenerate.sh:37
--plot-onlyis the only supported flag, but any other argument currently falls through and triggers the full benchmark run. That can be surprising (and expensive) if someone mistypes a flag or tries--help; it should fail fast on unknown options and on extra args.
if [[ "${1:-}" != "--plot-only" ]]; then
echo "==> Running statistical benchmarks (this takes a while)..."
BENCH_STATS=1 BENCH_STATS_OUT="$CSV" \
go test -v -run TestBenchStats -count=1 -timeout 180m .
fi
- Files reviewed: 3/15 changed files
- Comments generated: 0 new
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
Summary
The README's performance graphs only showed 5 of the 7 set implementations: the committed
bench_stats.csvpredatedSortedSetandBitSet, andbench_plot.pysilently dropped any implementation missing from its hardcodedIMPL_ORDERlist.benchmarks/regenerate.sh(new): one-step regeneration of the CSV and all charts. RunsTestBenchStatsacross every implementation registered inbenchImpls, so future set types are picked up automatically. Supports--plot-onlyto re-plot from the existing CSV, and falls back touv run --with matplotlibwhen matplotlib isn't installed.bench_plot.py: covers all 7 implementations; warns and plots (in black) any implementation found in the CSV but missing fromIMPL_ORDERinstead of silently dropping it; switches to a colorblind-safe categorical palette (validated: adjacent-pair CVD ΔE ≥ 8) with distinct per-series markers; y-axis is now log scale — the series span ~4 orders of magnitude (word-wiseBitSetops vs element-wise iteration), which a linear axis flattened into the x-axis.bench_stats.csvand all 11 charts from a fresh full run (sizes 10 → 1,000,000, 30 samples/op).BitSetappears in theintpanels only (integer-only implementation, as documented).Verification
./benchmarks/regenerate.sh --plot-onlyreproduces the committed charts from the committed CSV.implcolumn contains all 7 implementations; charts eyeballed — every series visible and distinguishable in both int and string panels.🤖 Generated with Claude Code
https://claude.ai/code/session_01VjUKQx2QYDuetR8z8GkGyU