Feature/multivariate wrapper

[JanFidor]

Fixes #1845 .

Summary

I've added a wrapper which allows a LocalForecastingModel to act like a GlobalForecastingModel

Other Information

FutureCovariatesLocalForecastingModel support multivariate series, but I decided to treat them like the rest of LocalForecastingModels and train separate instances of the model on each of the components of the provided series

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 86.79245% with 7 lines in your changes missing coverage. Please review.

Project coverage is 93.79%. Comparing base (a646adf) to head (e0eb50b).
Report is 35 commits behind head on master.

Files with missing lines	Patch %	Lines
...ecasting/multivariate_forecasting_model_wrapper.py	86.53%	7 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1917      +/-   ##
==========================================
- Coverage   93.80%   93.79%   -0.02%     
==========================================
  Files         139      140       +1     
  Lines       14714    14753      +39     
==========================================
+ Hits        13803    13837      +34     
- Misses        911      916       +5     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

---

🚨 Try these New Features:

• Flaky Tests Detection - Detect and resolve failed and flaky tests

[dennisbader]

Hi @JanFidor and thanks for this PR. #1845 is actually about a wrapper for multivariate series (a series with multiple target components / columns) rather than multiple series (multiple univariate or multivariate series in a list or sequence).

I don't think we will add a wrapper to make local models global, as this could potentially mean storing thousands of models. Also there would be a discrepancy between series used at training and prediction time.
The current global models train a single model on multiple series and accept any series at prediction time. For this wrapper, we don't know which trained model the series passed at prediction time should be mapped to.

[JanFidor]

Hi @dennisbader ! Thanks for the explanation, I can definitely see why a GlobalForecastingModel wrapper would be problematic. I made some fixes and the wrapper now operates on a single TimeSeries (univariate or multivariate) and changed the name accordingly

[madtoinou]

Cool addition, good work @JanFidor!

I noticed some left-over from the "multiple series support" implementation and EnsembleModel "template" :)

In addition of exposing this class to the users, we could maybe also include it in fit() logic as it's done for the RegressionModel in _fit_model() (model is wrapped in MultiOutputRegressor when necessary). Maybe with a warning since each sub-model will not be able to leverage all the components to generate their forecasts. WDYT @dennisbader ?

[JanFidor]

Again, thanks for the review @madtoinou ! You caught me red-handed, I was certain I didn't leave anything from the original "template" ; ) . Btw. it seems like something happened to StatsForecastAutoETS, because it breaks CI/CD on a few unrelated PRs (this one and #1915 ) included.

[madtoinou]

@JanFidor The error in the CI/CD was caused by a statsforacasting release where they fixed a bug that the unittest was actually "relying on", everything should run smoothly now.

I'll try to review your PR by the end of the weekend, thank you for addressing my points.

[madtoinou]

Hi @JanFidor,

A few additional comments about the docstring and simplification of the logic in fit() and predict().

[madtoinou]

Instead of the isinstance, rather use the supports_future_covariates.

Have a single call the predict with an inline if-else to pass the future_covariates (check suggestion in the fit()).

Also, it would be great to at least check that there is the same number of components in the forecasted series as models in the self_trained_models attribute.

[JanFidor]

Hi, I tried the approach with if else for passing the covariates, but it was throwing errors for models which didn't support future covariates

[madtoinou]

Because they were trained without the future_covariates (and hence theoretically support them but not in this context, at prediction time?) or because they actually did not support them?

[JanFidor]

Because they actually did not support them, some of the LocalForecastingModels don't support future covariates, so an error is thrown about future_covariates not being a parameter for fit() and predict()

[madtoinou]

Your tests are not matching the style of the others tests, please look how they are defined in test_local_forecasting_models.py (especially the parametrize decorator and the helper functions)

[felixdivo]

Should be fixed now (8c1b573)

[madtoinou]

Instead of loops in the test_fit_predict_local_models() and test_fit_predict_local_future_covariates_models() what will make the test fail if one of the model fails, without indicating which one, it would be great to leverage pytest.mark.parametrize and replace the list of instantiated models with a list of kwargs (one for local, one for local that supports future covariates), that would then be used to create the model before fit/predict (as done here).

Also, testing models that support future covariates without actually passing future covariates in fit() would be great.

[madtoinou]

Would be nice to check that local models trained manually, on each component of a series, does return the exact same forecasts as one in the wrapper.

[felixdivo]

This is now being tested.

[madtoinou]

Here, would be great to also leverage the pytest.mark.parametrize decorator and make the input series (univariate/multivariate) one of the parameter (in addition of the model, see comment above). So that if the test fail, it will be clear which series was involved.

[JanFidor]

Made the update, should be resolved!

[felixdivo]

Awesome that you takled this @JanFidor!

Do you plan to work on this further? I could, for example, help by adapting/extending the tests. I really want to soo this feature be completd since it makes my work quite a bit simpler. Do you want me to create a small PR to you branch JanFidor:feature/multivariate-wrapper for that?

[JanFidor]

Hi @felixdivo , thanks for the message! Long story short, this university semester had much more work than I anticipated so I went into survival mode for a bit hahaha. As soon as the semester ends (~3 weeks) I'm planning on going back to my old PRs and get them merged. If you feel up for writing some tests I'd be very thankful, but I'd still need to survive until the exams end to get the PR merged (I'm really sorry, but this month is especially terrible when it comes to university workload so there's no way I'll be able to get back to it any faster)

[felixdivo]

Hey @JanFidor, I feel you; this can definitely happen. I'll open a small PR patching a few things (like the tests, as advised by @madtoinou ), and you can just have a look once you're back from survival in creative mode. :) Feel free to reach out for help then. Good luck!

[felixdivo]

I'd maybe rename the model in the MultivariateForecastingModelWrapper to something like _template, since it is never actually used to make forecasts itself, but only as a blueprint to generate copies form.

[felixdivo]

@madtoinou are there unaddressed iossues in the PR? Maybe marking some comments above as resolved (if that is the case) helps maintain a better overview.

[madtoinou]

Closed the conversations that were addressed, the comments about pytest.mark.parametrize are still valid.

[felixdivo]

@JanFidor can you have a look at them?