#1101 Implemented min_train_series_length for Theta and FourTheta #1111
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
rijkvandermeulen
requested review from
hrzn,
tomasvanpottelbergh,
dennisbader and
brunnedu
as code owners
hrzn
approved these changes
Aug 7, 2022
| @@ -76,3 +76,51 @@ def test_best_model(self): | |||
| self.assertTrue( | |||
| mape(val_series, forecast_best) <= mape(val_series, forecast_random) | |||
| ) | |||
|
|
|||
| def test_min_train_series_length_with_seasonality(self): | |||
There was a problem hiding this comment.
Thank you for adding unit tests :)
Codecov Report
@@ Coverage Diff @@
## master #1111 +/- ##
==========================================
- Coverage 93.76% 93.75% -0.01%
==========================================
Files 80 80
Lines 8207 8204 -3
==========================================
- Hits 7695 7692 -3
Misses 512 512
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
hrzn
reviewed
Aug 8, 2022
CHANGELOG.md
Outdated
| @@ -34,6 +34,7 @@ Darts is still in an early development phase and we cannot always guarantee back | |||
| - An issue with arguments being reverted for the `metric` function of gridsearch and backtest [#989](https://github.com/unit8co/darts/pull/989) by [Clara Grotehans](https://github.com/ClaraGrthns). | |||
| - An error checking whether `fit()` has been called in global models [#944](https://github.com/unit8co/darts/pull/944) by [Julien Herzen](https://github.com/hrzn). | |||
| - An error in Gaussian Process filter happening with newer versions of sklearn [#963](https://github.com/unit8co/darts/pull/963) by [Julien Herzen](https://github.com/hrzn). | |||
| - Implemented the min_train_series_length method for the FourTheta and Theta models that overwrites the minimum default of 3 training samples by 2*seasonal_period when appropriate [#1101](https://github.com/unit8co/darts/pull/1101) by [Rijk van der Meulen](https://github.com/rijkvandermeulen) | |||
There was a problem hiding this comment.
@rijkvandermeulen here too, could you move this to the Unreleased section? Thanks.
There was a problem hiding this comment.
Jup - done!
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR implements the min_train_series_length() method for the FourTheta and Theta models that overwrites the minimum default of 3 training samples by 2*seasonal_period when appropriate.
Summary
When seasonality is used in the Theta and FourTheta models the minimum number of training samples should be 2*seasonal_period. Since the min_train_series_length() method wasn't implemented for the theta models, however, it always defaulted to 3 (for seasonal as well as non-seasonal models).
Other Information
Note that there is one case that the newly implemented min_train_series_length() method does not address: namely when season_mode != SeasonalityMode.NONE and seasonality_period==None. In this case, for us to be able to determine the minimum number of training samples, we need to first retrieve the seasonality_period from the time series. We can actually do this (for example by using the check_seasonality() function in utils.statistics). However, I wasn't entirely sure whether this would be desirable. The issue that I saw was that you lose your strict decoupling of models and time series. I.e., you cannot retrieve the min_train_series_length property from the model alone anymore - you'd have to pass the time series itself as well. Another possibility would be to make the seasonal_period a mandatory argument (probably not desired though I guess).
Looking forward to hearing your thoughts and feedback
Best,
Rijk