Skip to content

fix(backtest): normalize freq before querying benchmark feature data - #2348

Open
Pujitha Paladugu (pujitha24) wants to merge 1 commit into
microsoft:mainfrom
pujitha24:auto/issue-1925
Open

fix(backtest): normalize freq before querying benchmark feature data#2348
Pujitha Paladugu (pujitha24) wants to merge 1 commit into
microsoft:mainfrom
pujitha24:auto/issue-1925

Conversation

@pujitha24

Copy link
Copy Markdown

Description

get_higher_eq_freq_feature() (in qlib/utils/resam.py) now normalizes the
freq argument (e.g. "1day" -> "day") before querying D.features, so
that 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 raw
string is threaded through to PortfolioMetrics/Indicator and on into
_cal_benchmark() in qlib/backtest/report.py, which calls
get_higher_eq_freq_feature(..., freq="1day"). Feature bin files on disk are
named using the normalized freq string (see
FileFeatureStorage.__init__ in qlib/data/storage/file_storage.py, which
builds file_name from the raw freq, matching files like $close.day.bin).
Since "1day" and "day" are the same frequency but different strings, the
query for "1day" silently finds no data (it doesn't raise — the freq/support
check in FileStorageMixin.uri treats them as equal via Freq.__eq__, but
the literal filename lookup does not), and the resulting empty DataFrame
trips the len(_temp_result) == 0 check, surfacing as the misleading error
ValueError: The benchmark [...] does not exist. Please provide the right benchmark — even though the benchmark data is present under day freq.

Note this is a silently-empty-query / misleading-error bug, not a crash: the
existing try/except (ValueError, KeyError) fallback in
get_higher_eq_freq_feature was never reached here because no exception was
raised in the first place.

How Has This Been Tested?

  • Added a targeted regression test, tests/misc/test_resam.py, which
    mocks qlib.data.data.D and asserts get_higher_eq_freq_feature(freq="1day")
    queries with the normalized freq="day". Verified with 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 (includes the 2 new
    tests 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) 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 (unrelated to the fix).
  • Not run: the full CI matrix (make dev, full pytest tests/ against
    downloaded 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):

  1. Pipeline test: not run (see "Not run" note above).
  2. Your own tests: python -m pytest tests/misc/test_resam.py -v — 2 passed
    (fails on pre-fix code, passes after the fix).

Types of changes

  • Fix bugs
  • Add new feature
  • Update documentation

Fixes #1925

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)
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.

backtest occur errror while executor's time_per_step=1day

1 participant