Clean up gluonts.time_feature.holiday
#2578
Conversation
lostella
added
the
BREAKING
| NEW_YEARS_EVE: distance_to_holiday(NewYearsEve), | ||
| BLACK_FRIDAY: distance_to_holiday(BlackFriday), | ||
| CYBER_MONDAY: distance_to_holiday(CyberMonday), | ||
| "new_years_day": distance_to_holiday(NewYearsDay), |
There was a problem hiding this comment.
Some of them are referenced in the docstings below.
There was a problem hiding this comment.
I think we can remove that class entirely, it's only used in tests and I don't see any way the user can use it with estimators.
Instead, holidays should probably work just like any other TimeFeature?
There was a problem hiding this comment.
so if they are time features we just need to make sure they are not called on the fly as these operations are slow and in theory are just needed once and then every time series in a dataset can use them...
There was a problem hiding this comment.
Good point @kashif, some caching would help there I guess. In general I believe we should re-think how these features are specified to estimators, since now there seems to be too much going on.
lostella
changed the title
gluonts.time_feature.holidaygluonts.time_feature.holiday
Issue #, if available:
Description of changes:
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