Open pni-mft opened 1 year ago
Hi @pni-mft,
Thank you for contributing to darts.
Instead of sharing the code in an issue, can you please create a branch, save you code there and open a pull request? Github documentation contains step-by-step instructions to contribute to open source project here.
Since you're introducing a new model, it would be very valuable to also add some tests and examples to display the accuracy gain on some well known datasets compared to other existing models.
Hi @madtoinou,
Thanks for the feedback, I purposely did not put this in a pull request as I figured somebody closer to the development of the other forecasting model base classes would gave better insight in to why this functionality is not implemented in the TransferableFutureCovariatesLocalForecastingModel (support for past covariates) or in a similar base class. And if it were to be implemented how this should be done efficiently, and how to integrate some of the tests that already exist for the other base classes properly. It is not really an implementation of a new model, it just allows for users to extend the class in a way which supports passing of past covariates without having to go all the way and using a the RegressionModel base class.
There are many domain specific reasons why a extension of this base class would benefit from being able to access the past covariates as well when forecasting, but again that is not really relevant to this issue as it is primarily the addition of a base class which is the issue. Essentially I have a case where I am extending the forecasting model classes and wish to avoid using sklearn models and the overhead involved in the RegressionModel Base class.
It seems currently that the TransferableFutureCovariatesLocalForecastingModel base class is implemented to support things such as the ARIMA implementation etc, however it is common practice to extend these base classes to leverage the timeseries manipulation portion of darts while implementing domain specific non-statistical models.
As you probably noticed by their name, the models abstract classes correspond to the covariate support (and for torch-based models, also the dataset to use for training/inference) or wrap tightly around other libraries implementations.
I don't know if a lot of users would actually have a use case for this abstract class, which is neither regression-based nor deep learning based and using both past and future covariates. Unless a model introduced by a peer-reviewed article requires a new abstract class, darts will probably not add it.
WDYT @dennisbader?
I am a little confused by your response here. Would it not be beneficial to have extendable template classes (similar in spirit to e.g. sktime: https://github.com/sktime/sktime/tree/main/extension_templates) in order to facilitate implementation of all types of models in the future? and also to have a more standardized approach to how to implement new models supporting the various combinations of future covariates, past covariates etc.
I don't think a peer review article is required to understand the motivation behind this. In essence it is just splitting the respnsibility of the RegressionModel class up so that the functionality required by implenting sklearn models is held in one class, and the handling of past covariates in another class. It seems like more of a design practice or architectural question to me. To me it seems like a fairly standard design pattern to have base classes which encompass the variability of the framework (i.e. future covariates, past covariates).
It also took only fairly minor changes to the already existing classes to include this as an extendable base class. Most of the functionality required for an extendable past covariates base class is just being silenced in the FutureCovariates class.
I guess as a user I am saying that I found it confusing that I could extend the TransferableFutureCovariatesLocalForecastingModel with my own _predict and _fit methods in my subclass, but that if I was looking to extend a base class which supported past_covariates the only option seems to be to extend the RegressionModel class or to implement my own abstract class.
I guess what I am also saying is that sometimes it is also overkill to implement a statistical model if what you are really looking for is some elementary baseline models, and some of the motivation behind darts from what I understand is the implementation/separation of the past and future covariates which is fairly unique. Again based on your own article:
https://unit8.com/resources/time-series-forecasting-using-past-and-future-external-data-with-darts/
you could imagine a scenario where if you wanted to predict flooding events, then perhaps you would like to find previous flooding events and use some observations from the previous flooding events augmented with forecasted flooding events as a sort of non-linear forecasting model.
Again here the assumption would be that previous flooding events would be a better model for flow than recent observations. In an area where major rainfall/flash flood events occur rarely or at strange frequencies, a simple way would be just to take accumulated rainfall over a time period find similar periods historically and then use there observations to predict outcomes.
Basically the use case would be any type of forecasting where historical observed values are relvant in calculating the forecasted target variable, be it statistical or non-statistical.
EDIT: Just realized I switched over to my non-work account during this reply, apologies for any confusion!
As a follow up question and also as a bit of a TLDR:
If I were to submit a pull request with this code functionality, is there a preferred method of integrating this in?
Should some of the responsibilities of RegressionModel base class be moved to a class like this?
Would it make more sense just to change TransferableFutureCovariatesLocalForecastingModel to support past covariates instead of implementing an entirely separate class?
Some of these changes would probably be breaking or at least require some serious changes to some of the other base classes. Are there already tests somewhere relating to the correct way of slicing the past covariates that would make sense to make use of here, so that integrating this into the framework would be more smoooth?
and finally: @madtoinou before I begin forking and making pull requests I would imagine it would be good to scope out what you guys are looking for first, and what thoughts you have in terms of a good way of implementing this extendable base class
Hi @pfwnicks. To give some context here: we only have the LocalForecastingModel
(no covariates support) and FutureCovariatesLocalForecastingModel
(future covariates only support), because none of our models require any other support.
That being said, we would rather like to include such a class along with a model/algorithm that supports it, so that we are also incentivised to maintain it.
In the end, the covariates support of all our models could theoretically be covered by one base class governing the lags (like what we have for RegressionModels). This would be a huge refactor though..
Is your feature request related to a current problem? Please describe. I have created a based on the TransferableFutureCovariatesLocalForecastingModel which I needed to tweak slightly so that it used past covariates in its forecast. As far as I can see the only thing similar to this are the Regression models but they require sklearn and models etc in order to work.
Describe proposed solution I have therefore created a class similar to TransferableFutureCovariatesLocalForecastingModel which includes the possibility to use past covariates.
Describe potential alternatives Perhaps the TransferableFutureCovariatesLocalForecastingModel itself could be augmented to allow this possibility.
Additional context
Here is my sample code mostly based off of the TransferableFutureCovariatesLocalForecastingModel :