Conversation
run_backtest stamped every BacktestForecast with the last period of the full dataset, but the tail of that dataset is the held-out window the splits forecast. The cutoff therefore landed after the periods predicted from it, which inverts the horizon distance derived from it. Evaluation.create already reads train_set.period_range[-1]; the REST path now does too. chap eval --dry-run did not forward n_retrain to backtest, so it always trained once no matter what the real run would do. That left the retrain-on-historic-data path unexercised by the very run meant to smoke test it.
|
Reviewed read-only. Both fixes look correct and minimal — approving in spirit. Two notes on the description and one follow-up worth capturing.
|
2 participants
Two bugs found while reviewing #554. Both predate that PR, which only brought them into view; both are still on master.
last_train_periodrecorded the dataset end, not the train cutoffrun_backtestsetlast_train_period = dataset.period_range[-1], but the tail of that dataset is precisely the held-out window the splits forecast. On a 20-period dataset withn_periods=3, n_splits=2, stride=1the splits forecast periods 16-19 while everyBacktestForecastrow was stamped with period 19 — a training cutoff after the periods predicted from it.horizon_distanceinEvaluation.to_flatis derived from this field, so it came out wrong for every REST-created backtest.Evaluation.createalready usestrain_set.period_range[-1]; the REST path now matches, so the two entry points agree.chap eval --dry-runignoredn_retrainThe dry-run branch called
backtest(...)withoutn_retrain, so it always trained once regardless of--backtest-params.n-retrain. A dry run withn_retrain=2reported a clean smoke test having never touched the retrain-on-historic_datapath the real run takes — which is the kind of failure a dry run exists to surface.Tests
Both tests fail on master and pass with the fix:
test_run_backtest_records_the_train_cutoff_not_the_dataset_endasserts the invariant directly:last_train_periodmust precede every period forecast from it. On master it fails for every row.test_eval_cmd_dry_run_retrains_n_retrain_timescountstraincalls through a dry run withn_retrain=2; on master the count is 1.Both reuse existing fixtures (
p_seeded_engine,weekly_full_data) and the existing_patched_eval_chainhelper, which gains an optionaldatasetargument so a test can run the backtest against real data instead of aMagicMock.make lintandmake testare green (1487 passed, 122 skipped, 4 xfailed, 1 xpassed).