Skip to content

fix: DailyBatchSampler yields per-day cross-sections instead of contiguous slices - #2342

Open
tongflau-dongzhu wants to merge 2 commits into
microsoft:mainfrom
tongflau-dongzhu:fix/daily-batch-sampler-cross-day
Open

fix: DailyBatchSampler yields per-day cross-sections instead of contiguous slices#2342
tongflau-dongzhu wants to merge 2 commits into
microsoft:mainfrom
tongflau-dongzhu:fix/daily-batch-sampler-cross-day

Conversation

@tongflau-dongzhu

@tongflau-dongzhu tongflau-dongzhu commented Sep 6, 2026

Copy link
Copy Markdown

Implements the fix discussed in #2319.

What changed

  • DailyBatchSampler (qlib/contrib/model/pytorch_gats_ts.py) now computes each trading day's actual row positions by grouping the physical row indices on the datetime level, instead of slicing contiguous ranges (
    p.arange(idx, idx + count)) under the false assumption that one day's rows are contiguous.
  • TSDataSampler.get_index() docstring now states the contract explicitly: swaplevel() changes label order only; rows stay physically <instrument, datetime> — so callers cannot silently assume datetime-contiguity (the question raised in the issue thread).
  • Added ests/model/test_daily_batch_sampler.py (mock data source, no data download): each yielded batch covers exactly one trading day's full instrument cross-section; batches follow chronological order; len equals the number of batches (it previously returned the sample count, which mismatched the number of yielded batches).

Verification

  • Before: a batch from an instrument-major layout spans 2-3 different dates (standalone repro included in the issue thread).
  • After: each batch is exactly one day across all instruments; the union of all batches covers every physical row exactly once.

Fixes #2319

@tongflau-dongzhu
fix: DailyBatchSampler yields per-day cross-sections instead of conti…
9bd78b6 
…guous slices

TSDataSampler rows are physically instrument-major (<instrument, datetime>);
get_index() swaps the label order only, so one trading day's rows are not
contiguous. The old code sliced np.arange(idx, idx + count) by per-day counts,
producing batches that span several trading days of one instrument and drop
the other instruments for those days.

Compute each day's actual row positions via groupby over the datetime level
and document the row-order contract on get_index().

Fixes microsoft#2319
@tongflau-dongzhu tongflau-dongzhu mentioned this pull request Sep 6, 2026
Open
@tongflau-dongzhu

tongflau-dongzhu commented Sep 6, 2026

Copy link
Copy Markdown
Author

@microsoft-github-policy-service agree

@tongflau-dongzhu
fix: align GATs.predict with day-major sampler order; fix test fixtur…
f034ee4 
…e (get_index returns swapped view)

- DailyBatchSampler exposes index_order (physical rows in iteration order)
- GATs.predict aligns predictions via get_index()[index_order] — previously
  it assumed instrument-major iteration, which the sampler no longer does
- test fixture mirrors real get_index() (swaplevel view over instrument-major
  rows); groupby(level=0) now groups by datetime as intended

Addresses review feedback on microsoft#2319 / PR microsoft#2342.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

DailyBatchSampler in pytorch_gats_ts.py yields cross-day batches: TSDataSampler.get_index() swaps index labels but not row order

1 participant

@tongflau-dongzhu