Add NaiveMovingAverage baseline model #1557
Conversation
There was a problem hiding this comment.
Thank you for contributing to darts! The PR looks great, just a few small things to address in my opinion.
The rolling-window implementation looks nice, however the np.convolve method could probably have been used in order to obtain a one-liner and avoid the for loop (but at the cost of readability). I don't know which one of the two options is closer to darts' mentality, I'll let @dennisbader decide :)
Codecov ReportPatch coverage:
📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more Additional details and impacted files@@ Coverage Diff @@
## master #1557 +/- ##
==========================================
- Coverage 94.08% 94.06% -0.02%
==========================================
Files 125 125
Lines 11188 11277 +89
==========================================
+ Hits 10526 10608 +82
- Misses 662 669 +7
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
| return self.input_chunk_length | ||
|
|
||
| def __str__(self): | ||
| return f"Naive moving average model, with input_chunk_length={self.input_chunk_length}" |
There was a problem hiding this comment.
Might be convenient to use something more concise here, such as
| return f"Naive moving average model, with input_chunk_length={self.input_chunk_length}" | |
| return f"NaiveMovingAverage({self.input_chunk_length})" |
as it can appear for instance in plots' legends or other such places.
There was a problem hiding this comment.
Yep, making it code-like looks much sharper and to the point. I looked around the repo and it seems like most models share this style, but there are some outliers including other NaiveModels. What do you think about opening a new issue for bringing other models in line with it?
darts/tests/models/forecasting/test_local_forecasting_models.py
Outdated
Show resolved
Hide resolved
…darts into feature/moving-average
Co-authored-by: madtoinou <32447896+madtoinou@users.noreply.github.com>
|
Just wanted to say that I had an absolute blast contributing this PR, in big part thanks to your reviews, explanations and patience with the occasional workflow fail on black ; P . Thanks @madtoinou @hrzn ! |
That's great to hear! And your contribution is greatly appreciated 👍 thanks! |
* implement NaiveMovingAverage model * rename Moving Average Filter * black * add NaiveMovingAverage to test local * delete debug print * black * isort * fix __init__.py file import * rename window param and delete unnecessary check * fix MA test param * add docs changes and deterministic assertion * use more readable assert syntax * add loger to raise_if_not Co-authored-by: madtoinou <32447896+madtoinou@users.noreply.github.com> * black (again) --------- Co-authored-by: Julien Herzen <julien@unit8.co> Co-authored-by: madtoinou <32447896+madtoinou@users.noreply.github.com> Co-authored-by: Dennis Bader <dennis.bader@gmx.ch>
Fixes #951
Summary
I did a bit of research, and couldn't find anything that would support auto-regressive rolling window, so I implemented it from the ground up. I've decided to use numpy as the backbone of my implementation, main reasons being:
There are a few shortcomings with it, mainly iterating by index and using it to mutate the ndarray, which is a concatenation of the forecasts and the starting sliding window. Personally it feels a little iffy when it comes to clean code, but this approach saves us from an if-else block, which would be even less readable.
My first idea was to use a double linked list -> collections.deque for its popLeft() / append() methods and dynamically keeping a sliding window of forecasts. In the end I decided it wasn't a good idea to introduce this kind of algorithmic heavy approach out of the blue, so I shelved it and went with numpy