darts icon indicating copy to clipboard operation
darts copied to clipboard

Feature/multivariate wrapper

Open JanFidor opened this issue 2 years ago • 12 comments

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

JanFidor avatar Jul 25 '23 14:07 JanFidor

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.

codecov-commenter avatar Jul 25 '23 15:07 codecov-commenter

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.

dennisbader avatar Jul 31 '23 07:07 dennisbader

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

JanFidor avatar Aug 02 '23 11:08 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 https://github.com/unit8co/darts/pull/1915 ) included.

JanFidor avatar Aug 27 '23 11:08 JanFidor

@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 avatar Sep 06 '23 14:09 madtoinou

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?

felixdivo avatar Jan 09 '24 16:01 felixdivo

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)

JanFidor avatar Jan 09 '24 20:01 JanFidor

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 avatar Jan 11 '24 13:01 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 avatar Jan 12 '24 17:01 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.

felixdivo avatar Mar 03 '24 10:03 felixdivo

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

madtoinou avatar Mar 05 '24 10:03 madtoinou

@JanFidor can you have a look at them?

felixdivo avatar Mar 05 '24 14:03 felixdivo