fix: benchmark name shows as BENCHMARK in tearsheet#1014
Conversation
df_final["benchmark"] returns a new Series on each access, so assigning to .name on that throwaway object is a no-op. QuantStats never saw the asset name and fell back to uppercasing the column label, producing "BENCHMARK" in the report header instead of the actual symbol (e.g. "SPY"). Fix: capture the series once with .rename() before the qs.reports.html call, and pass that named series to both the primary and KDE-retry call sites. Adds test_tearsheet_quantstats_args.py with two regression tests that assert the correct name reaches qs.reports.html on both paths.
|
ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR refactors how benchmark series names are established in the ChangesBenchmark Series Naming and Validation
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Pylint (4.0.5)lumibot/tools/indicators.py************* Module pylintrc ... [truncated 67401 characters] ... ] Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
lumibot/tools/indicators.py (1)
1944-1950: ⚡ Quick win
_benchmark_seriesnot propagated to the metrics generation paths.Both the
_write_tearsheet_metrics_json_fallbackfallback (line 1944) and themetrics_json_fncall (line 1996) still passdf_final["benchmark"]— a Series withname="benchmark". If QuantStats uses the Series name to label the benchmark column in those outputs, those artifacts will still show"benchmark"/"BENCHMARK"instead of the asset symbol, inconsistent with the now-correct HTML tearsheet.
_benchmark_seriesis already in scope; the fix is mechanical.♻️ Proposed fix
- metrics_df = qs.reports.metrics( - df_final["strategy"], - df_final["benchmark"], + metrics_df = qs.reports.metrics( + df_final["strategy"], + _benchmark_series,- metrics_json_fn( - df_final["strategy"], - df_final["benchmark"], + metrics_json_fn( + df_final["strategy"], + _benchmark_series,Also applies to: 1994-2002
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lumibot/tools/indicators.py` around lines 1944 - 1950, The metrics generation paths still pass df_final["benchmark"] (a Series named "benchmark") to qs.reports.metrics and metrics_json_fn causing outputs to be labeled "benchmark"; replace those calls to use the already-in-scope _benchmark_series instead. Update the call in the _write_tearsheet_metrics_json_fallback path where metrics_df is created (qs.reports.metrics(..., df_final["benchmark"], ...)) and the metrics_json_fn invocation to pass _benchmark_series so QuantStats receives the correct benchmark Series name; leave all other args (risk_free_rate, custom_metrics, display flags) unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@lumibot/tools/indicators.py`:
- Around line 1944-1950: The metrics generation paths still pass
df_final["benchmark"] (a Series named "benchmark") to qs.reports.metrics and
metrics_json_fn causing outputs to be labeled "benchmark"; replace those calls
to use the already-in-scope _benchmark_series instead. Update the call in the
_write_tearsheet_metrics_json_fallback path where metrics_df is created
(qs.reports.metrics(..., df_final["benchmark"], ...)) and the metrics_json_fn
invocation to pass _benchmark_series so QuantStats receives the correct
benchmark Series name; leave all other args (risk_free_rate, custom_metrics,
display flags) unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7f939403-9bd4-4d5c-84da-1d6113328497
📒 Files selected for processing (2)
lumibot/tools/indicators.pytests/test_tearsheet_quantstats_args.py
df_final["benchmark"] is a Series named "benchmark", causing QuantStats to label tearsheet benchmark columns as "benchmark" instead of the actual asset name. Both the metrics_json_fn path and the fallback _write_tearsheet_metrics_json_fallback now use _benchmark_series, which is already renamed to the benchmark asset name at the call site.
2 participants
Summary
df_final["benchmark"]returns a new Series on each access, so assigning to.nameon that throwaway object is a no-op — QuantStats never saw the asset name and fell back to uppercasing the column label, showing BENCHMARK in the tearsheet header instead of the actual symbol (e.g. SPY)..rename()and passes it to both the primary and KDE-retryqs.reports.htmlcall sites.tests/test_tearsheet_quantstats_args.pywith two regression tests that assert the correct name reachesqs.reports.htmlon both paths. Tests were verified to fail against the buggy code and pass with the fix.Test plan
uv run pytest tests/test_tearsheet_quantstats_args.py -v— both tests pass'benchmark'instead of symbol name)test_tearsheet.py,test_tearsheet_quantstats_retry_unit.py)Summary by CodeRabbit
Bug Fixes
Tests