[ENH] Add preprocessing (scalers and normalizers) to tslib datamodule - #2368
[ENH] Add preprocessing (scalers and normalizers) to tslib datamodule#2368echo-xiao wants to merge 14 commits into
Conversation
Add test_no_scalers_leaves_data_untouched to guard backward compatibility: when no scalers are configured, continuous features pass through untouched and target_scale is absent from the preprocessed output.
|
hi, @phoeenniixx please review these and let me know if these decisions are right:
TODO:
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2368 +/- ##
=======================================
Coverage ? 90.43%
=======================================
Files ? 203
Lines ? 11090
Branches ? 0
=======================================
Hits ? 10029
Misses ? 1061
Partials ? 0
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
|
||
|
|
||
| def _make_ts(n_series: int = 20, length: int = 40, offset: float = 100.0) -> TimeSeries: | ||
| """合成数据集:连续特征 ``x`` 远离 0(~offset),便于看出标准化;目标 ``y`` 是正弦。""" |
There was a problem hiding this comment.
please use english docstrings
phoeenniixx
left a comment
There was a problem hiding this comment.
Nice!
I have a few questions:
- Did you look at the
thumlimplementation? I think we should try to keep it as close to this package as possible - when it comes to the preprocessing logic as this dm is based on this package- Currently it supports all the scalers and normalizers that
EncoderDecoderDataModulesupports, but is this true for thetslibimplementation as well?
- Currently it supports all the scalers and normalizers that
Also, please use english docstrings, to maintain uniformity across the documentation.
Address review feedback on sktime#2368: the helper docstring and inline comments added for the preprocessing tests were written in Chinese. Rewrite them in English to stay consistent with the rest of the codebase documentation.
`TslibDataModule` accepted `scalers` and `target_normalizer` but never used them. This makes them functional, fit once on the training split, mirroring thuml's data layer. Scope follows thuml's preprocessing, which hardcodes a single global sklearn StandardScaler fit on the train segment: - Only sklearn `StandardScaler` is accepted. Anything else raises `NotImplementedError` rather than failing deep inside the scaler stack or, in the case of `EncoderNormalizer`, silently producing wrongly scaled data (its `fit_per_sequence` semantics are inverted by a global fit). - `target_normalizer` now defaults to `None` instead of `"auto"`, and `"auto"` resolves to no normalization. The inverse transform (`target_scale`) is not implemented yet (sktime#2359), and all three models on this data module guard on `"target_scale" in x`, so normalizing by default would leave every prediction in normalized space with no way back. Making the fit boundary hold required two further changes: - `_ensure_split` computes the train/val/test split once and caches it, matching what sktime#2302 did for `EncoderDecoderTimeSeriesDataModule`. `setup` runs once per stage, so the unconditional `torch.randperm` handed `trainer.test()` a different split than `trainer.fit()` had used, moving series the scalers were fit on into the test set. - The fit calls moved out of the `stage == "fit"` branch, so a standalone `setup("test")` no longer passes raw values through unscaled. Also drops the per-series preprocessing cache added earlier on this branch: it is unrelated to scaling and belongs with the base class work in sktime#2313. Drive-by: `test_multivariate_target` asserted `y.shape[-1] == 2`, but sktime#1960 changed multi-target `__getitem__` to return one tensor per target. The assertion is updated to match. This file is not collected by CI, which is why the failure went unnoticed.
`testpaths` listed only `tests/` and `pytorch_forecasting/tests/`, so `pytorch_forecasting/data/tests/` was never collected. Its tests have therefore not run in CI at all, which is why the codecov patch report shows 0% for `test_tslib_data_module.py` and why a stale assertion in `test_multivariate_target` went unnoticed for months. Adding the directory collects 34 further tests (1165 -> 1199) and they all pass, so this does not turn CI red.
`test_tslib_data_module.py` lived in `pytorch_forecasting/data/tests/`, which is not listed in `testpaths`, so it had never run in CI. That is why codecov reported 0% patch coverage for it, and why a stale assertion in `test_multivariate_target` (left behind by sktime#1960) went unnoticed. Move it to `tests/test_data/`, where every other data layer test already lives -- `test_data_module.py`, `test_d1.py`, `test_encoders.py`, `test_samplers.py`, `test_timeseries.py` -- and where sktime#2302 put the `EncoderDecoderTimeSeriesDataModule` scaler tests this PR mirrors. This also reverts the `testpaths` entry added in the previous commit, which is no longer needed. The 34 tests are now collected and all pass.
In thuml's data layer, it's preprocessing is:
tslib does not support EncoderDecoderDataModule supports.
what this pr does:
|
`TslibDataModule` now rejects normalizers other than sklearn's `StandardScaler`, so the tutorial's `target_normalizer=TorchNormalizer()` raises `NotImplementedError` and fails the notebook CI job. Switch the tutorial to `StandardScaler` and drop the now unused `TorchNormalizer` import.
|
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
Reference Issues/PRs
Fixes #2330.
What does this implement/fix? Explain your changes.
TslibDataModuleacceptedscalers/target_normalizerbut never used them(stored and ignored). This makes them functional, following #2302 and reusing
ScalerAdapter:__init__wraps them inScalerAdapter, adds_*_fittedflags and aper-series
_preprocess_cache._fit_scalers/_fit_target_normalizerfit on the train split only._normalize_features/_normalize_targettransform per series (no-op untilfitted; only configured continuous columns).
_preprocess_dataapplies the transforms and caches per series;setup("fit")fits on_train_indices.What should a reviewer concentrate their feedback on?
target_scale** to a follow-upDid you add any tests for the change?
Yes — 7 tests in
pytorch_forecasting/data/tests/test_tslib_data_module.py:_preprocess_datascaling + cache identity;setup("fit")produces scaled samples;target_scale).PR checklist
pre-commit run --fileson both changed files, clean).