Skip to content

[DO NOT MERGE] Experimental Design for new v2 API - #2398

Draft
phoeenniixx wants to merge 5 commits into
sktime:mainfrom
phoeenniixx:ptf-new-design
Draft

[DO NOT MERGE] Experimental Design for new v2 API#2398
phoeenniixx wants to merge 5 commits into
sktime:mainfrom
phoeenniixx:ptf-new-design

Conversation

@phoeenniixx

@phoeenniixx phoeenniixx commented Aug 27, 2026

Copy link
Copy Markdown
Member

This PR tries a new prototype for new design.
Created _proto to have the files with major changes and some minor changes (like to EncoderDecoderDM) has been made in place.

NOTE

This is a prototype - not to be merged as here the idea is just to get a minimal working state of the design to test out how it feels. I have done somethings which will not happen in ACTUAL implementation. Like

  • BaseForecaster is inheriting from BasePkg which will not happen in actuality as BaseFroecaster will REPLACE BasePkg.
  • Special nb for this prototype - will replace the current tutorials
  • _proto folder with Forecaster, TimeSeries_datatype etc files. which will be present in their designated folders and not in this folder
  • TimeSeries_datatype will be renamed to TimeSeries if we decide to move on with this plan

What should a reviewer concentrate their feedback on?

Just the notebook and try out the vignettes and play around with it. This PR is not meant for the "code review" but just for the review of the vignettes and design

Try the notebook on colab (i have added a cell to install the branch as well)

AI Generated, so it SHOULD NOT BE MERGED
Only raised to try out the new interface and get feedback. If approved, will be implemented over a stack of PRs :)

@phoeenniixx

Copy link
Copy Markdown
Member Author

I still need to add forecaster etc. I have already added what i think will be added to this PR in the description. I willl update the desc if something changes

@phoeenniixx phoeenniixx self-assigned this Aug 28, 2026
@review-notebook-app

Copy link
Copy Markdown

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@CloseChoice

CloseChoice commented Sep 2, 2026

Copy link
Copy Markdown

I think this design is good. I played around with it in your notebook and that is fine.

I just got issues when trying to play with this from scratch:

from pytorch_forecasting._proto._timeseries_datatype import TimeSeries_datatype as TimeSeries

tft = TFTForecaster()
ts = TimeSeries(data_df,
                time="time_idx")
tft.fit(ts)

This resulted in an error:

---------------------------------------------------------------------------
ValueError                                Traceback (most recent call last)
Cell In[5], line 6
      2 
      3 tft = TFTForecaster()
      4 ts = TimeSeries(data_df,
      5                 time="time_idx")
----> 6 tft.fit(ts)

File ~/programming/pytorch-forecasting/pytorch_forecasting/_proto/_base_forecaster.py:135, in BaseForecaster.fit(self, data, trainer)
    131 self.model_ = self.get_cls()(
    132     **self.model_cfg, metadata=self.datamodule_.metadata
    133 )
    134 self.trainer_ = self._resolve_trainer(trainer)
--> 135 self.trainer_.fit(self.model_, datamodule=self.datamodule_)
    137 self._is_fitted = True
    138 return self

File ~/programming/pytorch-forecasting/.venv/lib/python3.13/site-packages/lightning/pytorch/trainer/trainer.py:584, in Trainer.fit(self, model, train_dataloaders, val_dataloaders, datamodule, ckpt_path, weights_only)
    582 self.training = True
    583 self.should_stop = False
--> 584 call._call_and_handle_interrupt(
    585     self,
    586     self._fit_impl,
    587     model,
    588     train_dataloaders,
    589     val_dataloaders,
    590     datamodule,
    591     ckpt_path,
    592     weights_only,
    593 )

File ~/programming/pytorch-forecasting/.venv/lib/python3.13/site-packages/lightning/pytorch/trainer/call.py:49, in _call_and_handle_interrupt(trainer, trainer_fn, *args, **kwargs)
     47     if trainer.strategy.launcher is not None:
     48         return trainer.strategy.launcher.launch(trainer_fn, *args, trainer=trainer, **kwargs)
---> 49     return trainer_fn(*args, **kwargs)
     51 except _TunerExitException:
     52     _call_teardown_hook(trainer)

File ~/programming/pytorch-forecasting/.venv/lib/python3.13/site-packages/lightning/pytorch/trainer/trainer.py:630, in Trainer._fit_impl(self, model, train_dataloaders, val_dataloaders, datamodule, ckpt_path, weights_only)
    623     download_model_from_registry(ckpt_path, self)
    624 ckpt_path = self._checkpoint_connector._select_ckpt_path(
    625     self.state.fn,
    626     ckpt_path,
    627     model_provided=True,
    628     model_connected=self.lightning_module is not None,
    629 )
--> 630 self._run(model, ckpt_path=ckpt_path, weights_only=weights_only)
    632 assert self.state.stopped
    633 self.training = False

File ~/programming/pytorch-forecasting/.venv/lib/python3.13/site-packages/lightning/pytorch/trainer/trainer.py:1079, in Trainer._run(self, model, ckpt_path, weights_only)
   1074 self._signal_connector.register_signal_handlers()
   1076 # ----------------------------
   1077 # RUN THE TRAINER
   1078 # ----------------------------
-> 1079 results = self._run_stage()
   1081 # ----------------------------
   1082 # POST-Training CLEAN UP
   1083 # ----------------------------
   1084 log.debug(f"{self.__class__.__name__}: trainer tearing down")

File ~/programming/pytorch-forecasting/.venv/lib/python3.13/site-packages/lightning/pytorch/trainer/trainer.py:1123, in Trainer._run_stage(self)
   1121         self._run_sanity_check()
   1122     with torch.autograd.set_detect_anomaly(self._detect_anomaly):
-> 1123         self.fit_loop.run()
   1124     return None
   1125 raise RuntimeError(f"Unexpected state {self.state}")

File ~/programming/pytorch-forecasting/.venv/lib/python3.13/site-packages/lightning/pytorch/loops/fit_loop.py:209, in _FitLoop.run(self)
    208 def run(self) -> None:
--> 209     self.setup_data()
    210     if self.skip:
    211         return

File ~/programming/pytorch-forecasting/.venv/lib/python3.13/site-packages/lightning/pytorch/loops/fit_loop.py:238, in _FitLoop.setup_data(self)
    235 log.debug(f"{self.__class__.__name__}: resetting train dataloader")
    237 source = self._data_source
--> 238 train_dataloader = _request_dataloader(source)
    239 trainer.strategy.barrier("train_dataloader()")
    241 if not isinstance(train_dataloader, CombinedLoader):

File ~/programming/pytorch-forecasting/.venv/lib/python3.13/site-packages/lightning/pytorch/trainer/connectors/data_connector.py:335, in _request_dataloader(data_source)
    324 """Requests a dataloader by calling dataloader hooks corresponding to the given stage.
    325 
    326 Returns:
    327     The requested dataloader
    328 
    329 """
    330 with _replace_dunder_methods(DataLoader, "dataset"), _replace_dunder_methods(BatchSampler):
    331     # under this context manager, the arguments passed to `DataLoader.__init__` will be captured and saved as
    332     # attributes on the instance in case the dataloader needs to be re-instantiated later by Lightning.
    333     # Also, it records all attribute setting and deletion using patched `__setattr__` and `__delattr__`
    334     # methods so that the re-instantiated object is as close to the original as possible.
--> 335     return data_source.dataloader()

File ~/programming/pytorch-forecasting/.venv/lib/python3.13/site-packages/lightning/pytorch/trainer/connectors/data_connector.py:302, in _DataLoaderSource.dataloader(self)
    300 if isinstance(self.instance, pl.LightningDataModule):
    301     assert self.instance.trainer is not None
--> 302     return call._call_lightning_datamodule_hook(self.instance.trainer, self.name)
    303 assert self.instance is not None
    304 return self.instance

File ~/programming/pytorch-forecasting/.venv/lib/python3.13/site-packages/lightning/pytorch/trainer/call.py:199, in _call_lightning_datamodule_hook(trainer, hook_name, *args, **kwargs)
    197 if callable(fn):
    198     with trainer.profiler.profile(f"[LightningDataModule]{trainer.datamodule.__class__.__name__}.{hook_name}"):
--> 199         return fn(*args, **kwargs)
    200 return None

File ~/programming/pytorch-forecasting/pytorch_forecasting/data/data_module/_encoder_decoder_data_module.py:1190, in EncoderDecoderTimeSeriesDataModule.train_dataloader(self)
   1189 def train_dataloader(self):
-> 1190     return DataLoader(
   1191         self.train_dataset,
   1192         batch_size=self.batch_size,
   1193         num_workers=self.num_workers,
   1194         shuffle=True,
   1195         collate_fn=self.collate_fn,
   1196     )

File ~/programming/pytorch-forecasting/.venv/lib/python3.13/site-packages/lightning/fabric/utilities/data.py:325, in _wrap_init_method.<locals>.wrapper(obj, *args, **kwargs)
    322     elif store_explicit_arg in kwargs:
    323         object.__setattr__(obj, f"__{store_explicit_arg}", kwargs[store_explicit_arg])
--> 325 init(obj, *args, **kwargs)
    326 object.__setattr__(obj, "__pl_inside_init", old_inside_init)

File ~/programming/pytorch-forecasting/.venv/lib/python3.13/site-packages/lightning/fabric/utilities/data.py:325, in _wrap_init_method.<locals>.wrapper(obj, *args, **kwargs)
    322     elif store_explicit_arg in kwargs:
    323         object.__setattr__(obj, f"__{store_explicit_arg}", kwargs[store_explicit_arg])
--> 325 init(obj, *args, **kwargs)
    326 object.__setattr__(obj, "__pl_inside_init", old_inside_init)

File ~/programming/pytorch-forecasting/.venv/lib/python3.13/site-packages/torch/utils/data/dataloader.py:401, in DataLoader.__init__(self, dataset, batch_size, shuffle, sampler, batch_sampler, num_workers, collate_fn, pin_memory, drop_last, timeout, worker_init_fn, multiprocessing_context, generator, prefetch_factor, persistent_workers, pin_memory_device, in_order)
    399 else:  # map-style
    400     if shuffle:
--> 401         sampler = RandomSampler(dataset, generator=generator)  # type: ignore[arg-type]
    402     else:
    403         sampler = SequentialSampler(dataset)  # type: ignore[arg-type]

File ~/programming/pytorch-forecasting/.venv/lib/python3.13/site-packages/torch/utils/data/sampler.py:149, in RandomSampler.__init__(self, data_source, replacement, num_samples, generator)
    144     raise TypeError(
    145         f"replacement should be a boolean value, but got replacement={self.replacement}"
    146     )
    148 if not isinstance(self.num_samples, int) or self.num_samples <= 0:
--> 149     raise ValueError(
    150         f"num_samples should be a positive integer value, but got num_samples={self.num_samples}"
    151     )

ValueError: num_samples should be a positive integer value, but got num_samples=0

Sure, I did not specify the arguments for TimeSeries_datatype properly, but this error message does not help me at all. I also think this should raise an error on creation time of the time series (seems like group is the missing parameter here!)

I would also think that when doing

ds = tft.predict(ts)

the resulting columns should be exactly named as the input cols when doing

ds.to_pandas()

currently we get:

Screenshot From 2026-09-02 13-47-43

But I understand that these are secondary issues.

@phoeenniixx

Copy link
Copy Markdown
Member Author

Thanks for the review @CloseChoice!

  • For the errors - yes, the error handling is not good rn, but tbh i didnt focus that much on it, as it can be added once we actually implement the classes
  • For the predictions. I think we currently lack the complete implementation of the inverse transforms even, I agree we should have the exact format as the input, this can be easily handled via TimSeries_datatype now. We just need to implement this, it is not present in the current branch

The current branch is just a Proof of concept, and if you all like it then we can create complete workstreams and implement each feature step - by - step

"outputs": [],
"execution_count": null,
"source": [
"!pip install git+https://github.com/phoeenniixx/pytorch-forecasting.git@ptf-new-design"

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

general best practice suggestion: jupyter notebooks should not modify the runtime environment...

@phoeenniixx phoeenniixx Sep 4, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, this was intentionally added so that people can run this notebook direclty in colab as well if they want to. As we are not going to merge this, I thought it provides the reviewer some flexibility (not setting up ptf in their env) and then pulling the branch.

@fkiraly fkiraly left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice! I think this is great!

I have one question though:

  • why is TimeSeries no longer a Dataset? Is there anything that forces us to move away from standard APIs resp classes? I do not see anything that we could not do with a dataset

@phoeenniixx

phoeenniixx commented Sep 4, 2026

Copy link
Copy Markdown
Member Author
  • why is TimeSeries no longer a Dataset? Is there anything that forces us to move away from standard APIs resp classes? I do not see anything that we could not do with a dataset

I think Dataset signifies something more - a data conatiner - and not datatype. And the user should not think that it is another dataset layer, but as a datatype. The dataset makes me think something that handles data and not some otehr representation of data. This data set is never used actually as it is passed to dm for further operations. The thing is - we dont need TimeSeries to be a dataset. It being dataset doesnt add anything over what we have rn as a class, (we can still have __getitem__, etc) but adds some complexity (inheritance)

I also think in future, we can refactor this datatype to be more "generic" (removing torch or any other dep completely) and be used for "communication" between packages (like sktime and ptf - or maybe even inbetween sktime as well). But the complete idea is still not consolidated in my mind.

Also, we already have a Private dataset class for each Datamodules, and if we loose anything on not having TimeSeries as Dataset (which i think is None) - we get it anyway via the private dataset classes.

What do you think? should we move back to it being Dataset?

@phoeenniixx
phoeenniixx requested a review from fkiraly September 4, 2026 17:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants