[ENH] Add neuralforecast adapter and rnn forecaster#5962
[ENH] Add neuralforecast adapter and rnn forecaster#5962fkiraly merged 3 commits intosktime:mainfrom yarnabrina:add-neuralforecast-adapter-and-rnn-forecaster
Conversation
|
@AzulGarza @jmoralez if you can please take a look at this PR and share your reviews for this adaptation, especially if you have suggestions that will help in supporting a long term stable adapter, that will be much appreciated. |
yarnabrina
left a comment
There was a problem hiding this comment.
Most of CI is disabled to quickly get test results for new addition. This is not part of the main PR goal, and will be reverted before merge. Ignore all changes in .github.
NOTE: revert 2b756d9 before marking ready for merge.
|
FYI @fkiraly @benHeid @achieveordie : all tests pass for the relevant adapter and estimators in all environment combinations (CI is modified to ensure all tests for these run on all combinations of operating systems and python versions). Other than addressing reverting CI debug commit just before merge, I have no other new changes to add in this PR (other than review comments of course). |
fkiraly
left a comment
There was a problem hiding this comment.
Looks nice!
Few questions:
- the in-sample failures can be silenced by raising
NotImplementedErrorifnot fh_is_all_in_sample(). @benHeid had the same issue recently, probably we should do that in the base class. - question: how would we test this in the intended end state?
- a bit unsettling that
freqis an argument of__init__, rather than obtained fromXorfh. Is the latter not possible?
1. added generic adapter 2. added concrete RNN estimator 3. added tests 4. added reference in docs 5. updated `dl` dependency
jmoralez
left a comment
There was a problem hiding this comment.
LGTM on the nixtla side. Thanks for working on this!
Adapter class to
neuralforecastfromNixtlaand concrete estimator forRNN.Notes for reviewers (not in any particular order)
neuralforecastsupports 3 types of exogenous features: static (constant over time), historical (unknown for prediction horizon) and future (known for prediction horizon). I felt the static type has no predictive capability if models are trained per series, and as far as I know, sktime will not be able to support historical type for framework compatibilities. Hence in this first draft of adaptation, I have added support for only future type.neuralforecasthas the capability of directly supporting panel/hierarchical datasets directly. I am not sure how to take benefit of that in sktime, so have not used it in this first draft. If it's easy enough to support, I can update if some references are shared, otherwise this can definitely be a extension.RNNinneuralforecastsupports specification of loss and validation loss, but they needs to be instances of pytorch loss functions. I am not sure how set these defaults in instance, as soft dependencies can not be checked there. I have hence not exposed these parameters and rely on the defaults, but this is both important and common parameter. With some suggestions and references, I will like to enhance this in this draft itself.neuralforecastdoes support probablistic forecasts, but it seems that depends on choice of loss function (and may be something more, not sure). In this first draft, I have not added that support so far and this can be part of an extension.neuralforecasthas a publicNeuralForecastclass which acceptspandas.DataFrameobjects of special schema directly, and has the ability to train multiple models and find best one. The underlying models do not supportpandas.DataFramedirectly and relies ofTimeSeriesDataset, a sub-class oftorch.utils.data.Dataset. For simplification with most other forecasters, I opted for the higher level class.neuralforecastsupports in-sample predictions, but warns that those may be inaccurate. Since the library itself is marked asAlpha, I have disabled entire support of in-sample predictions for now.neuralforecasttakes a lot of dependencies, and adding it inforecastingextra may cause too much installation for users who won't use it. But adding it indlextra means new CI is not going to test it, so we need to decide what to do.neuralforecastsupports a lot of models, and (hopefully) all of them can be used with the adapter in this PR. However that is a lot of models, and it leads to the question of how should we organise them, by model or by dependency or by both or by something else.dldependency similar to other recent PR's that affect torch/tensorflow etc.