Feature/multivariate wrapper
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 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).
| Files | Patch % | Lines |
|---|---|---|
| ...ecasting/multivariate_forecasting_model_wrapper.py | 86.53% | 7 Missing :warning: |
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
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
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.
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
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 https://github.com/unit8co/darts/pull/1915 ) included.
@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.
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?
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)
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!
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.
@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.
Closed the conversations that were addressed, the comments about pytest.mark.parametrize are still valid.
@JanFidor can you have a look at them?