fix(backtest): keep excess_return consistent with corrected benchmark_return - #1058
Merged
warren618 merged 1 commit intoAug 11, 2026
Merged
Conversation
…_return BaseEngine.run_backtest() overwrites the displayed benchmark_return with bench_result.total_ret, a price relative computed the HKUDS#872-safe way once an external benchmark's price series contains a non-positive-prior-price bar (suspended stock, vendor glitch). excess_return is never recomputed: it stays derived from calc_metrics()'s internal bench_return, which is still (1 + bench_ret).prod() - 1, the exact compounded-product pattern HKUDS#872 already found unsafe and fixed for the displayed field. The result is a self-contradictory metrics dict: benchmark_return reads the correct value while excess_return implies a different one, both in the same JSON output the backtest tool returns. Recompute excess_return from the same corrected benchmark_return right where benchmark_metadata already overwrites the other field, so the two can never disagree again. Only applies when an external benchmark was actually resolved; the internal-basket-benchmark path (no config["benchmark"]) is untouched. Signed-off-by: Lanre Shittu <136805224+Shizoqua@users.noreply.github.com>
warren618
added a commit
that referenced
this pull request
Aug 11, 2026
#1058 re-derived excess_return from the corrected benchmark total but left it unrounded, and m.update(benchmark_metadata) had already replaced the rounded benchmark_return with a raw float. calc_metrics() rounds both fields to 6 decimals, so the external-benchmark path was emitting a different precision style for the same keys into JSON and CSV output. Both fields now round exactly the way calc_metrics() does -- rounding the result, not the inputs -- so the corrected path and the default path are indistinguishable in shape.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Why
Issue #872 replaced the fragile pct_change-based return calculation with a safe price relative (buy_and_hold_return) for the displayed benchmark_return, after finding it could report numbers like +39,560% where the true return was -42.7%. calc_metrics() has its own internal re-derivation of the same total, still using the exact (1 + bench_ret).prod() - 1 pattern #872 found unsafe. base.py corrects the displayed field but never re-derives excess_return from the corrected value, so the metrics dict can become self-contradictory: benchmark_return reads correctly while excess_return implies a different number, both in the same JSON output the backtest tool returns to the LLM.
I audited every benchmark-derived metric in calc_metrics() (benchmark_return, excess_return, information_ratio, tracking_error, benchmark_beta). Only excess_return is affected. The other three are computed from the per-bar bench_ret series directly, which is already the corrected, #872-safe series by the time it reaches calc_metrics().
Changes
Test Plan
Checklist