Repository navigation
&Faakhir30 [ENH] Update Datamodules according to new API design - #2423
phoeenniixx wants to merge 3 commits into
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## ts #2423 +/- ##
==========================================
+ Coverage 87.79% 87.95% +0.15%
==========================================
Files 202 203 +1
Lines 11292 11373 +81
==========================================
+ Hits 9914 10003 +89
+ Misses 1378 1370 -8
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:
|
|
The |
|
|
||
| self.save_hyperparameters(ignore=["loss", "logging_metrics", "optimizer"]) | ||
| self.save_hyperparameters( | ||
| ignore=["loss", "logging_metrics", "optimizer", "metadata"] |
There was a problem hiding this comment.
need to add metadata to be ignored to prevent the torch UnpicklingError as metadata is now a dataclass
Wire the v2 batch format through to ``MultiLoss`` so multi-target models can
train, and fail early with an actionable message when the loss and the data
disagree about how many targets there are.
``MultiLoss`` indexes ``y_pred[idx]``, ``y_actual[0][idx]`` and ``y_actual[1]``,
but the v2 datamodules emit ``y`` as a bare ``list[Tensor]``. Passing that
through unchanged made ``MultiLoss`` read a batch row as a target and the
second target as a sample weight - either a confusing shape error deep inside
the loss, or a silently wrong loss value.
* add ``_coerce_y_for_loss`` / ``_coerce_y_hat_for_loss``, which adapt targets
and predictions to the layout the loss expects and raise on a count mismatch
* factor the three stage methods onto a shared ``_step``
* support ``MultiLoss`` in ``to_prediction`` / ``to_quantiles``, removing the
two ``# todo: add MultiLoss support`` markers
* compute logging metrics per target, tagged ``target{idx}_``
* read ``self._loss`` rather than ``self.loss`` for every ``MultiLoss`` check,
so an ``NNLossAdapter``-wrapped loss takes the right branch
Deliberately narrow: sample weights, datamodule changes and the
``PredictCallback`` collate needed for multi-target ``predict()`` are left for
follow-ups. Tests use synthetic batches rather than datamodules so the pinned
contract survives the datamodule rework in sktime#2423.
Supersedes the model half of sktime#2324; the coercion design is Aryan's.
Two deviations from sktime#2324 worth review:
* its ``to_prediction``/``to_quantiles`` took ``use_metric=False`` as default,
routing through the unbound ``Metric.to_prediction`` and bypassing subclass
overrides. ``QuantileLoss`` and ``NNLossAdapter`` both override it, and v1
defaults this flag to ``True``, so that would have silently changed quantile
predictions from the median to the mean of the quantiles. This keeps the
bound call and lets ``MultiLoss.to_prediction`` do its own per-target
iteration, which it already implements.
* its stacked-prediction split had no rank check, so a ``[batch, horizon]``
output under a two-metric ``MultiLoss`` with ``horizon == 2`` would slice
time steps instead of targets. Splitting now requires ``ndim >= 3``.
Co-authored-by: Aryan Saini <116151399+phoeenniixx@users.noreply.github.com>
This reverts commit 663a893.
Co-authored-by: Faakhir30 <zahidfaakhir@gmail.com>
CloseChoice
left a comment
There was a problem hiding this comment.
This looks fine but there are a couple problems, I added them below
| time_series=basic_timeseries_dataset, | ||
| batch_size=2, | ||
| context_length=12, | ||
| prediction_length=8, |
There was a problem hiding this comment.
here's a problem codex found and I verified:
from pathlib import Path
from tempfile import TemporaryDirectory
import lightning
import torch
from _common import make_module, make_timeseries
from pytorch_forecasting.metrics import MAE
from pytorch_forecasting.models.samformer._samformer_v2 import Samformer
module = make_module(make_timeseries())
model = Samformer(
loss=MAE(), hidden_size=4, use_revin=False, metadata=module.metadata
)
print("Metadata saved in hyperparameters:", "metadata" in model.hparams)
# Minimal Lightning checkpoint: no training is needed to exercise reconstruction.
checkpoint = {
"state_dict": model.state_dict(),
"hyper_parameters": dict(model.hparams),
"pytorch-lightning_version": lightning.__version__,
}
with TemporaryDirectory(prefix="dm-preview-") as directory:
path = Path(directory) / "samformer.ckpt"
torch.save(checkpoint, path)
Samformer.load_from_checkpoint(path, loss=MAE())returns an error.
| time_series=basic_timeseries_dataset, | ||
| batch_size=2, | ||
| context_length=12, | ||
| prediction_length=8, |
There was a problem hiding this comment.
another problem
TorchNormalizer can be trained on one dim and applied to multiple.
"""Show that two-target prediction data can reuse a one-target normalizer."""
import warnings
import numpy as np
import pandas as pd
import torch
from pytorch_forecasting.data.data_module import EncoderDecoderTimeSeriesDataModule
from pytorch_forecasting.data.encoders import TorchNormalizer
from pytorch_forecasting.data.timeseries import TimeSeries
def make_timeseries(offset=0.0, num_groups=10, targets=None):
groups = np.repeat(np.arange(num_groups), 40)
times = np.tile(np.arange(40), num_groups)
frame = pd.DataFrame(
{
"group": groups,
"time": times,
"target": offset + 10.0 + times + groups,
"second_target": offset + 100.0 + times * 2 + groups,
"feature": times.astype(float),
}
)
return TimeSeries(
frame,
time="time",
target=targets or ["target"],
group=["group"],
num=["feature"],
known=["feature"],
)
def make_module(data=None):
torch.manual_seed(0)
with warnings.catch_warnings():
warnings.filterwarnings("ignore", message=".*experimental.*")
return EncoderDecoderTimeSeriesDataModule(
time_series=data,
max_encoder_length=10,
max_prediction_length=5,
target_normalizer=TorchNormalizer(),
)
trained = make_module(make_timeseries())
trained.setup("fit")
prediction = trained.with_data(
make_timeseries(targets=["target", "second_target"])
)
prediction.setup("predict")
x, _ = next(iter(prediction.predict_dataloader()))
print("Training target count:", trained.n_targets)
print("Prediction target count:", prediction.n_targets)
print("Prediction target_past shape:", tuple(x["target_past"].shape))
assert trained.n_targets == 1 and prediction.n_targets == 2
assert prediction._target_normalizer is trained._target_normalizer
print("BUG reproduced: incompatible targets are accepted and share the fitted normalizer.")| time_series=basic_timeseries_dataset, | ||
| batch_size=2, | ||
| context_length=12, | ||
| prediction_length=8, |
There was a problem hiding this comment.
also here is another bug, fitting a second config mutates state of the first. I think these should be handled separately.
"""Show that fitting a second configuration copy mutates the first copy."""
import torch
import warnings
import numpy as np
import pandas as pd
from pytorch_forecasting.data.data_module import EncoderDecoderTimeSeriesDataModule
from pytorch_forecasting.data.encoders import TorchNormalizer
from pytorch_forecasting.data.timeseries import TimeSeries
def make_timeseries(offset=0.0, num_groups=10, targets=None):
groups = np.repeat(np.arange(num_groups), 40)
times = np.tile(np.arange(40), num_groups)
frame = pd.DataFrame(
{
"group": groups,
"time": times,
"target": offset + 10.0 + times + groups,
"second_target": offset + 100.0 + times * 2 + groups,
"feature": times.astype(float),
}
)
return TimeSeries(
frame,
time="time",
target=targets or ["target"],
group=["group"],
num=["feature"],
known=["feature"],
)
def make_module(data=None):
torch.manual_seed(0)
with warnings.catch_warnings():
warnings.filterwarnings("ignore", message=".*experimental.*")
return EncoderDecoderTimeSeriesDataModule(
time_series=data,
max_encoder_length=10,
max_prediction_length=5,
target_normalizer=TorchNormalizer(),
)
config = make_module()
first = config.with_data(make_timeseries())
first.setup("fit")
center_before = first._target_normalizer._scaler.center_.clone()
second = config.with_data(make_timeseries(offset=1000.0))
second.setup("fit")
center_after = first._target_normalizer._scaler.center_.clone()
print("First module's center before second fit:", center_before.item())
print("First module's center after second fit:", center_after.item())
assert first._target_normalizer is second._target_normalizer
assert not torch.equal(center_before, center_after)
print("BUG reproduced: fitting the second module changes the first module's statistics.")
``| time_series=basic_timeseries_dataset, | ||
| batch_size=2, | ||
| context_length=12, | ||
| prediction_length=8, |
There was a problem hiding this comment.
This could be one of the most serious bugs, there is an overlap between train and test overlap:
"""Show that single-series training and evaluation use identical windows."""
import warnings
import numpy as np
import pandas as pd
import torch
from pytorch_forecasting.data.data_module import EncoderDecoderTimeSeriesDataModule
from pytorch_forecasting.data.encoders import TorchNormalizer
from pytorch_forecasting.data.timeseries import TimeSeries
def make_timeseries(offset=0.0, num_groups=10, targets=None):
groups = np.repeat(np.arange(num_groups), 40)
times = np.tile(np.arange(40), num_groups)
frame = pd.DataFrame(
{
"group": groups,
"time": times,
"target": offset + 10.0 + times + groups,
"second_target": offset + 100.0 + times * 2 + groups,
"feature": times.astype(float),
}
)
return TimeSeries(
frame,
time="time",
target=targets or ["target"],
group=["group"],
num=["feature"],
known=["feature"],
)
def make_module(data=None):
torch.manual_seed(0)
with warnings.catch_warnings():
warnings.filterwarnings("ignore", message=".*experimental.*")
return EncoderDecoderTimeSeriesDataModule(
time_series=data,
max_encoder_length=10,
max_prediction_length=5,
target_normalizer=TorchNormalizer(),
)
module = make_module(make_timeseries(num_groups=1))
module.setup("fit")
module.setup("test")
print("Train indices:", module._train_indices.tolist())
print("Validation indices:", module._val_indices.tolist())
print("Test indices:", module._test_indices.tolist())
assert module.train_windows
assert module.train_windows == module.val_windows == module.test_windows
print(f"BUG reproduced: all splits contain the same {len(module.train_windows)} windows.")
Stacks on #2410 (and #2313 in coming commits FYI @Faakhir30 )
Also see #2407, #2398
This PR is aimed at
v2-devinstead ofmain, so that we dont break current user code until this design is complete and stable. See #2407 for details. So I had to update the branch target in the workflows for the tests to run (which might fail due to the changes)Stack
TimeSeries- [ENH] AddTimeSeriesdatatype as per new API #2410Datamodule- &Faakhir30 [ENH] Update Datamodules according to new API design #2423 <- This PRForecaster- [ENH] Addforecastersupport and remove_pkgclass for new API #2434Note
The tutorials will fail for sure, will update the notebook in one of the branchs stacked over this PR.
Still need to add the
Forecasterand tests - will add the notebook and docs changes on the upcoming branches as the vignette is incomplete withoutforecasterwhich is not in this PR.Changes
EncDecDataModuleMetadata,TimeSeriesMetadataetc) are added to_metadata.pyutils/_validationmetadatainignoreofsave_hyperparametersofSamformer. See this commentEncDecDataModuleMetadata(in case ofEncoderDecoderTImeSeriesDatamodule) and tslib dm has its own metadata dataclass.with_datamethod to create a new dm instance using the already present params (like already fitted scalers - only to create the transformed data).forecaster.fit()while we accept the dm class in__init__of theforecaster. So, the class is actually just empty at this point without data.