Feature/multivariate wrapper #1917

JanFidor · 2023-07-25T14:56:58Z

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 · 2023-07-25T15:47:14Z

Codecov Report

Patch coverage: 87.03% and project coverage change: -0.02% ⚠️

Comparison is base (137cf8c) 93.72% compared to head (ddc3fd3) 93.70%.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1917      +/-   ##
==========================================
- Coverage   93.72%   93.70%   -0.02%     
==========================================
  Files         131      132       +1     
  Lines       12662    12702      +40     
==========================================
+ Hits        11867    11902      +35     
- Misses        795      800       +5

Files Changed	Coverage Δ
...ecasting/multivariate_forecasting_model_wrapper.py	`86.79% <86.79%> (ø)`
darts/models/__init__.py	`58.22% <100.00%> (+0.53%)`	⬆️

... and 7 files with indirect coverage changes

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

dennisbader · 2023-07-31T07:12:57Z

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.

…l wrapper

JanFidor · 2023-08-02T11:06:49Z

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 ?

darts/models/forecasting/multivariate_forecasting_model_wrapper.py

JanFidor · 2023-08-27T11:28:41Z

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 · 2023-09-06T14:11:34Z

@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().

darts/models/forecasting/multivariate_forecasting_model_wrapper.py

darts/timeseries.py

darts/models/forecasting/multivariate_forecasting_model_wrapper.py

madtoinou · 2023-10-20T09:15:03Z

darts/models/forecasting/multivariate_forecasting_model_wrapper.py

+        predictions = [
+            model.predict(n=n, future_covariates=future_covariates)
+            if isinstance(model, FutureCovariatesLocalForecastingModel)
+            else model.predict(n=n)
+            for model in self._trained_models


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.

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

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?

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()

darts/models/forecasting/multivariate_forecasting_model_wrapper.py

madtoinou · 2023-10-20T09:17:35Z

darts/tests/models/forecasting/test_multivariate_forecasting_model_wrapper.py

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)

Should be fixed now (8c1b573)

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 · 2023-10-20T09:18:37Z

darts/tests/models/forecasting/test_multivariate_forecasting_model_wrapper.py

+                model, self.n_pred, combination, future_covariates
+            )
+            assert isinstance(preds, TimeSeries)
+            assert preds.n_components == combination.n_components


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.

This is now being tested.

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.

felixdivo · 2024-01-09T16:22:38Z

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 · 2024-01-09T20:53:47Z

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 · 2024-01-11T13:53:31Z

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 · 2024-01-12T17:16:57Z

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.

Improve testing code for MultivariateForecastingModelWrapper

…ate-wrapper # Conflicts: # darts/timeseries.py

Co-authored-by: madtoinou <32447896+madtoinou@users.noreply.github.com>

felixdivo · 2024-03-03T10:48:12Z

@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 · 2024-03-05T10:45:11Z

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

felixdivo · 2024-03-05T14:14:55Z

@JanFidor can you have a look at them?

JanFidor added 3 commits July 25, 2023 16:42

add global_forecasting_model_wrapper.py

83bba23

add tests

08999fc

add comments

a079c8e

JanFidor requested a review from dennisbader as a code owner July 25, 2023 14:56

Merge branch 'master' into feature/multivariate-wrapper

ef4d305

JanFidor and others added 2 commits August 2, 2023 13:00

Switch to being a multivariate model wrapper instead of a global mode…

d652e3a

…l wrapper

Merge branch 'unit8co:master' into feature/multivariate-wrapper

ddc3fd3

madtoinou requested changes Aug 22, 2023

View reviewed changes

JanFidor added 4 commits August 25, 2023 22:53

Merge branch 'master' into feature/multivariate-wrapper

c2e7037

refactor

ba39d55

add missing property decorator

48cb156

update tests

fb1c06b

madtoinou added 2 commits September 6, 2023 16:11

Merge branch 'master' into feature/multivariate-wrapper

450e081

Merge branch 'master' into feature/multivariate-wrapper

36d3f13

madtoinou requested changes Oct 20, 2023

View reviewed changes

Improve testing code for MultivariateForecastingModelWrapper

5a0e32a

felixdivo mentioned this pull request Jan 12, 2024

Improve testing code for MultivariateForecastingModelWrapper JanFidor/darts#1

Merged

JanFidor and others added 4 commits February 5, 2024 21:31

Merge pull request #1 from felixdivo/feature/multivariate-wrapper

8c1b573

Improve testing code for MultivariateForecastingModelWrapper

Merge remote-tracking branch 'upstream/master' into feature/multivari…

908cfd4

…ate-wrapper # Conflicts: # darts/timeseries.py

Expand description

bf64cff

Co-authored-by: madtoinou <32447896+madtoinou@users.noreply.github.com>

Refactor fit loop to make it more intuitive

aea93b1

Co-authored-by: madtoinou <32447896+madtoinou@users.noreply.github.com>

JanFidor added 2 commits February 5, 2024 22:35

delete function and rewrite docstring

05af0ca

fix future_covariates error

37226d3

Merge branch 'master' into feature/multivariate-wrapper

2b4138a

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/multivariate wrapper #1917

Feature/multivariate wrapper #1917

JanFidor commented Jul 25, 2023

codecov-commenter commented Jul 25, 2023 •

edited

dennisbader commented Jul 31, 2023

JanFidor commented Aug 2, 2023

madtoinou left a comment

JanFidor commented Aug 27, 2023

madtoinou commented Sep 6, 2023

madtoinou left a comment

madtoinou Oct 20, 2023

JanFidor Feb 7, 2024

madtoinou Feb 8, 2024

JanFidor Feb 8, 2024

madtoinou Oct 20, 2023

felixdivo Feb 8, 2024

madtoinou Feb 8, 2024

madtoinou Oct 20, 2023

felixdivo Feb 8, 2024

madtoinou Feb 8, 2024

felixdivo commented Jan 9, 2024

JanFidor commented Jan 9, 2024

felixdivo commented Jan 11, 2024

felixdivo commented Jan 12, 2024

felixdivo commented Mar 3, 2024

madtoinou commented Mar 5, 2024

felixdivo commented Mar 5, 2024

Feature/multivariate wrapper #1917

Are you sure you want to change the base?

Feature/multivariate wrapper #1917

Conversation

JanFidor commented Jul 25, 2023

Summary

Other Information

codecov-commenter commented Jul 25, 2023 • edited

Codecov Report

dennisbader commented Jul 31, 2023

JanFidor commented Aug 2, 2023

madtoinou left a comment

Choose a reason for hiding this comment

JanFidor commented Aug 27, 2023

madtoinou commented Sep 6, 2023

madtoinou left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

felixdivo commented Jan 9, 2024

JanFidor commented Jan 9, 2024

felixdivo commented Jan 11, 2024

felixdivo commented Jan 12, 2024

felixdivo commented Mar 3, 2024

madtoinou commented Mar 5, 2024

felixdivo commented Mar 5, 2024

codecov-commenter commented Jul 25, 2023 •

edited