-
Notifications
You must be signed in to change notification settings - Fork 744
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Remove freq
argument in SeasonalNaivePredictor
#2932
Conversation
season_length: Optional[int] = None, | ||
imputation_method: Optional[ | ||
MissingValueImputation | ||
] = LastValueImputation(), | ||
) -> None: | ||
super().__init__(prediction_length=prediction_length) | ||
|
||
assert (freq is not None) or ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure this would be nicer.
maybe.or_(season_length, freq).expect("You must provide one of `freq` or `season_length`")
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the use of freq
, if season_length
is passed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's ignored (season_length
takes precendence); another option is that not both can be specified
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure this would be nicer.
not sure, I find the explicit assertion clearer
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would keep the option of both parameters to be given, with season_length
taking precedence.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I actually like removing freq
as a parameter and require setting season_length
(why is it not called seasonality
?).
That way things become much more explicit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe I am wrong, but with this modification this model would be the only one that explicitly requires the parameter season_length
, and I am not sure this is something we want. I agree that giving priority to season_length
is something nice, but in many cases one computes seasonal_naive
just as a reference without giving so much thought about the seasonality. For instance, in our metrics we use calculate_seasonal_error
and I am not sure to what degree this would then also require an explicit value for season_length
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but with this modification this model would be the only one that explicitly requires the parameter season_length
I think that’s fine. The only thing common to all models really is that they have a fixed prediction length; in general, I think it’s fine that models each have their own set of options, related to how they work internally.
This model has a very simple logic, entirely determined by season_length
: specifying this directly has no ambiguity. Adding indirection to translate freq
only hides a little bit what the model will end up doing, with no added flexibility.
Then there are the choices for the default seasonality for hourly and daily data, which are completely arbitrary and end up tying together the behavior of different models or components for no reason.
In summary, I tend to agree that if you want to use the naive seasonal method, you should bring your own seasonality parameter. We do provide some heuristics to get that from frequency, and one is free to use them, but should do that explicitly.
Removed |
freq
argument optional in SeasonalNaivePredictor
freq
argument in SeasonalNaivePredictor
I think naive2 could be simplified in a similar way, see gluonts/src/gluonts/ext/naive_2/_predictor.py Lines 61 to 80 in 3ccb6d3
|
@lostella Do you want to adjust naive2 in a separate PR? |
Description of changes: So far,
freq
was a mandatory argument in constructingSeasonalNaivePredictor
. However, its role is not really clear this way: what is it used for? Can the predictor only treat data sampled at frequencyfreq
? No, the frequency information is only used to infer the seasonality in case not directly provided.This PR fixes this by removing
freq
altogether: nowseason_length
is required.By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.
Please tag this pr with at least one of these labels to make our release process faster: BREAKING, new feature, bug fix, other change, dev setup