[Chetansahney]

Reference Issues/PRs

No existing issue. The shared split came to BaseTimeSeriesDataModule in #2423.

Related: #2087 (open, against main) moves splitting into compute_split_boundaries, which rounds instead of truncating. With the default split that still gives 3 series no validation series, and gives 4 or 5 series no test series, so the same guard applies there. Happy to carry it over when #2087 and v2-dev meet.

What does this implement/fix? Explain your changes.

BaseTimeSeriesDataModule._ensure_split sizes the train and validation sets with int(). With the default (0.7, 0.15, 0.15), int(0.15 * n) is 0 for any dataset with 3 to 6 series, so validation gets no series, while 1 and 2 series are special-cased and do get one:

series	train / val / test before	after
3	2 / 0 / 1	1 / 1 / 1
4	2 / 0 / 2	2 / 1 / 1
5	3 / 0 / 2	3 / 1 / 1
6	4 / 0 / 2	4 / 1 / 1
7 or more	unchanged	unchanged

With an empty validation set Lightning skips validation without a warning, so the default ModelCheckpoint(monitor="val_loss") in Base_pkg.fit() never saves anything. Fitting DecoderMLP_pkg_v2 on 4 series, the size of the v2 test data, logs only train_loss, and fit() returns Path(".") as the best checkpoint. The (0.8, 0.2) split the v2 test suite uses behaves the same way.

When validation rounds to zero, this takes one series from the test remainder, which holds the rounding surplus, or from train when test cannot spare one; test keeps at least one series whenever its fraction is positive. Splits that already give validation a series are unchanged, so with the default split, datasets with 7 or more series are split exactly as before.

What should a reviewer concentrate their feedback on?

• Taking the validation series from the test surplus first. With 4 series the old split put 2 of the 4 series in test against a requested 15%, so 2 / 1 / 1 is closer to 0.7 / 0.15 / 0.15 than taking it from train, which would give 1 / 1 / 2.
• A two-entry split such as (0.8, 0.2) has no test fraction, so test can give up its last series: 4 series split 3 / 1 / 0.

Did you add any tests for the change?

test_few_series_split_keeps_validation in tests/test_data/test_data_module.py checks the exact split sizes for 3 to 7 series and for (0.8, 0.2), for both EncoderDecoderTimeSeriesDataModule and TslibDataModule, that every series lands in exactly one split, and that the validation dataset is not empty. 10 of the 12 cases fail before the change; the two 7-series cases, which already had a validation series, pass both ways.

tests/test_data and the full v2 estimator suite in pytorch_forecasting/tests/test_all_v2 (240 tests) pass. The v2 test data has 4 series, so those integration tests now run validation, which they did not before.

Any other comments?

Three related things I noticed and left out of this PR:

1. When no checkpoint is saved, Base_pkg.fit() returns Path(checkpoint_cb.best_model_path), which is Path("."), and _save_artifact then writes datamodule_cfg.pkl, model_cfg.pkl and metadata.pkl into the current working directory. [ENH] Add forecaster support and remove _pkg class for new API #2434 and [ENH] Add minimal vignette tests for v2 design and fix default datamodule compatibility #2475 both rework _base_pkg.py, so I did not touch it here.
2. Now that a checkpoint is saved, test_integration writes it to ./checkpoints (gitignored), because _integration calls pkg.fit() with the default relative ckpt_dir. Passing ckpt_dir=tmp_path would keep it in the test's temp directory; _test_integration.py is also being changed in [ENH] Add minimal vignette tests for v2 design and fix default datamodule compatibility #2475.
3. pytorch_forecasting/data/tests/test_tslib_data_module.py fails 14 of 14 on v2-dev: it still passes time_series_dataset=, which &Faakhir30 [ENH] Update Datamodules according to new API design #2423 renamed to time_series=. CI does not collect pytorch_forecasting/data/tests (see [MNT] run in-package test folders in CI, fix test_multivariate_target #2478), which is why this went unnoticed. Happy to update it in a follow-up.

PR checklist

• The PR title starts with either [ENH], [MNT], [DOC], or [BUG].
• Added/modified tests
• Used pre-commit hooks when committing to ensure that code is compliant with hooks.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
⚠️ Please upload report for BASE (v2-dev@4c9d6bd). Learn more about missing BASE report.

Additional details and impacted files

@@            Coverage Diff            @@
##             v2-dev    #2479   +/-   ##
=========================================
  Coverage          ?   88.06%           
=========================================
  Files             ?      203           
  Lines             ?    11394           
  Branches          ?        0           
=========================================
  Hits              ?    10034           
  Misses            ?     1360           
  Partials          ?        0           

Flag	Coverage Δ
cpu	88.06% <100.00%> (?)
pytest	88.06% <100.00%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.