Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[ENH] re-interface API for LTSF algorithms #6192

Closed
geetu040 opened this issue Mar 22, 2024 · 6 comments
Closed

[ENH] re-interface API for LTSF algorithms #6192

geetu040 opened this issue Mar 22, 2024 · 6 comments
Labels
enhancement Adding new functionality module:forecasting forecasting module: forecasting, incl probabilistic and hierarchical forecasting

Comments

@geetu040
Copy link
Contributor

LTSF algorithms were originally requested in #4939

Following forecasters and accessories were added by @luca-miniati in #4891 and 9c6a956

  • forecasters sktime.forecasting.ltsf
    • LTSFLinearForecaster
    • LTSFDLinearForecaster
    • LTSFNLinearForecaster
  • networks sktime.networks.ltsf._ltsf
    • LTSFLinearNetwork
    • LTSFDLinearNetwork
    • LTSFNLinearNetwork
  • adapters: BaseDeepNetworkPyTorch

LTSF algorithms are now supported in neuralforecast (said here: cure-lab/LTSF-Linear) for which we have an existing adapter

My proposition is that we change the internals of LTSF API, basically change its adapter from BaseDeepNetworkPyTorch to _NeuralForecastAdapter.

Reasons for this change are:

  • we can completely remove sktime.networks.ltsf._ltsf as we will be importing the networks implemented in neuralforecast
  • the architechture for LTSF algorithms also happen to change with enhacements, with the new API design we will not have to worry about changing the networks in sktime.networks.ltsf._ltsf
  • our code size will reduce by approximately 150 lines per algorithm
  • it would be easier to interface other ltsf algorithms like: Transformer, Informer, Autoformer, Pyraformer, FEDformer. It would be a plug and play as we would not need to implement the complete architectures from scratch

Steps to start with the change

  • decide with the deprecation of existing LTSF interface
    • either start a deprecation cycle
    • or keep the existing interface and implement a new one
  • decide the location of new ltsf interface
    • new ltsf interface can be implemented in sktime/forecasting/neuralforecast.py
    • or we can create a new file sktime/forecasting/ltsf2.py for the deprecation period and later make it the main interface by removing 2 from filename
  • implement the new interface just like NeuralForecastRNN and NeuralForecastLSTM have been already implemented
@geetu040 geetu040 added the enhancement Adding new functionality label Mar 22, 2024
@fkiraly fkiraly added the module:forecasting forecasting module: forecasting, incl probabilistic and hierarchical forecasting label Mar 22, 2024
@fkiraly
Copy link
Collaborator

fkiraly commented Mar 22, 2024

Good observation!

I would go with the "implement a new one" plan.

Why: afaik, the current implementations are "reference implementations", taken from the cited repositories (which have not been released as packages on pypi).

So, they have - at least - an important place in comparisons, benchmarks, etc, for scientific purposes. Further, I'd imagine they are not fully interface identical with the neuralforecast ones (I have not checked though).

Regarding the name, I would add as an option, perhaps we don't necessarily want to switch them, but keep neuralforecast in the name, to give credit to neuralforecast / nixtla brand. So there's the original "academic" implementation which has been a reference for a while at least, and there's the nixtla implementation, which may or may not be improved, optimized, etc.

What do you think?

@geetu040
Copy link
Contributor Author

sure, I'll check how the original academic implemention and that of neuralforecast and sktime differs

@fkiraly
Copy link
Collaborator

fkiraly commented Mar 22, 2024

hm, looks like nixtla did the same thing and copied the code? That's a curious situation then.

If it is a 1:1 copy - like in sktime - I see no reason to "outsource" it to nixtla?

Because accessing the same code behind an additional layer of interfaces makes it more brittle - which in the case of nixtla historically comes with substantial breakage risk (@yarnabrina probably has opinions on this, as the creator and maintainer of the statsforecast facing one).

Further, nixtla packages tend to take dependencies on all of pypi (exaggerating a little), while sktime minimizes dependencies. This would make the dependency requirements, for users of the same estimator, worse.

@geetu040
Copy link
Contributor Author

hm, looks like nixtla did the same thing and copied the code? That's a curious situation then.

yes, I was checking DLinear and it does look quite the same

Further, nixtla packages tend to take dependencies on all of pypi (exaggerating a little), while sktime minimizes dependencies. This would make the dependency requirements, for users of the same estimator, worse.

Yes, just observed the same thing, was working on nixtla on colab and it takes really long to install all the packages unlike sktime.

Because accessing the same code behind an additional layer of interfaces makes it more brittle - which in the case of nixtla historically comes with substantial breakage risk

I think this and the above reasons are strong enough to keep LTSF network implementations inside sktime

@fkiraly
Copy link
Collaborator

fkiraly commented Mar 22, 2024

perhaps we should make the original authors aware that their algorithms exist in sktime and are indexed?

I am also noticing, they do not appear in the author tag, even though the code is copied. So, as an action, we should add the GitHub IDs of the original authors?

(this is my oversight, we added author/maintainer tags for estimators recently and we added them to 100s of estimators using default settings for the migration, based on file authors. Also see policy discussion in #5938 )

@geetu040
Copy link
Contributor Author

I am working on adding an LTSF algorithm, so I'll fix the author tag.

I am closing the issue, as we have had a good discussion and have reached a conclusion to not change the API design.

@geetu040 geetu040 closed this as not planned Won't fix, can't repro, duplicate, stale Mar 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Adding new functionality module:forecasting forecasting module: forecasting, incl probabilistic and hierarchical forecasting
Projects
None yet
Development

No branches or pull requests

2 participants