fix(backtest): normalize freq before querying benchmark feature data - #2348
Open
Pujitha Paladugu (pujitha24) wants to merge 1 commit into
Open
fix(backtest): normalize freq before querying benchmark feature data#2348Pujitha Paladugu (pujitha24) wants to merge 1 commit into
Pujitha Paladugu (pujitha24) wants to merge 1 commit into
Conversation
Motivation: When a backtest executor is configured with time_per_step="1day", that raw string is threaded through PortfolioMetrics/Indicator into _cal_benchmark() in qlib/backtest/report.py, which calls get_higher_eq_freq_feature(..., freq="1day") in qlib/utils/resam.py. Feature files on disk are named using the canonical/normalized freq string (e.g. "day"), so a query for the equivalent but differently spelled "1day" silently returns no data instead of finding the existing "day" data. The resulting empty result trips a len(_temp_result) == 0 check and surfaces as a misleading error: "ValueError: The benchmark [...] does not exist. Please provide the right benchmark", even though the benchmark data is present. This is a silently-empty-query / misleading-error bug, not a crash: the existing try/except fallback in get_higher_eq_freq_feature is never reached since no exception was raised in the first place. Approach: Normalize `freq` via `str(Freq(freq))` at the top of get_higher_eq_freq_feature, before it is used to query D.features. This maps "1day" -> "day" (and leaves already-canonical freqs like "day"/"1min"/"5min" unchanged) so the query matches the freq string actually used to name feature files on disk. Validation: - Added tests/misc/test_resam.py, a targeted regression test that mocks qlib.data.data.D and asserts freq="1day" is normalized to "day" before querying D.features. Verified via git stash that this test fails on the pre-fix code (AssertionError: '1day' != 'day') and passes after the fix. - cd tests && python -m pytest misc/ -v: 13 passed, no regressions. - black qlib/utils/resam.py tests/misc/test_resam.py -l 120 --check --diff: clean. - pylint (repo's configured ignore list) on qlib/utils/resam.py: 10.00/10. - flake8 (repo's configured ignore list) on qlib/utils/resam.py: clean. - mypy qlib/utils/resam.py: 2 pre-existing errors reported, both on lines untouched by this change. - Not run: the full CI matrix (full pytest suite against downloaded CN market data, sphinx docs, nbconvert) - impractical to run in this environment. The fix is a small, self-contained string-normalization change fully exercised by the targeted unit test above without needing live market data. Report: microsoft#1925 Signed-off-by: Pujitha Paladugu <10557236+pujitha24@users.noreply.github.com> Assisted-by: claude-sonnet-5 (via Claude Code)
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.
Description
get_higher_eq_freq_feature()(inqlib/utils/resam.py) now normalizes thefreqargument (e.g."1day"->"day") before queryingD.features, sothat a freq string equivalent to but spelled differently than the freq used
to name feature files on disk still finds the data.
Motivation and Context
Fixes #1925
When a backtest executor is configured with
time_per_step="1day", that rawstring is threaded through to
PortfolioMetrics/Indicatorand on into_cal_benchmark()inqlib/backtest/report.py, which callsget_higher_eq_freq_feature(..., freq="1day"). Feature bin files on disk arenamed using the normalized freq string (see
FileFeatureStorage.__init__inqlib/data/storage/file_storage.py, whichbuilds
file_namefrom the raw freq, matching files like$close.day.bin).Since
"1day"and"day"are the same frequency but different strings, thequery for
"1day"silently finds no data (it doesn't raise — the freq/supportcheck in
FileStorageMixin.uritreats them as equal viaFreq.__eq__, butthe literal filename lookup does not), and the resulting empty DataFrame
trips the
len(_temp_result) == 0check, surfacing as the misleading errorValueError: The benchmark [...] does not exist. Please provide the right benchmark— even though the benchmark data is present underdayfreq.Note this is a silently-empty-query / misleading-error bug, not a crash: the
existing
try/except (ValueError, KeyError)fallback inget_higher_eq_freq_featurewas never reached here because no exception wasraised in the first place.
How Has This Been Tested?
tests/misc/test_resam.py, whichmocks
qlib.data.data.Dand assertsget_higher_eq_freq_feature(freq="1day")queries with the normalized
freq="day". Verified withgit stashthatthis test fails on the pre-fix code (
AssertionError: '1day' != 'day') andpasses after the fix.
cd tests && python -m pytest misc/ -v— 13 passed (includes the 2 newtests plus all pre-existing tests in that directory; no regressions).
black qlib/utils/resam.py tests/misc/test_resam.py -l 120 --check --diff— clean.pylint(repo's configured ignore list) onqlib/utils/resam.py— 10.00/10.flake8(repo's configured ignore list) onqlib/utils/resam.py— clean.mypy qlib/utils/resam.py— 2 pre-existing errors reported, both on linesuntouched by this change (unrelated to the fix).
make dev, fullpytest tests/againstdownloaded CN market data, sphinx docs, nbconvert) — this requires
downloading several GB of market data and was impractical to run in this
environment. The change is a small, self-contained string-normalization fix
fully exercised by the targeted unit test above without needing live market
data.
Screenshots of Test Results (if appropriate):
python -m pytest tests/misc/test_resam.py -v— 2 passed(fails on pre-fix code, passes after the fix).
Types of changes
Fixes #1925