-
Notifications
You must be signed in to change notification settings - Fork 473
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
[minor] add subdivisions to holidays #1584
[minor] add subdivisions to holidays #1584
Conversation
chnage the country parameter to accept a dict or str to list and omit the new added parameter! |
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.
merge get_country_holidays
into get_holidays_from_country
and move to hdays_utils
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.
Looking good overall! I just suggest to move the get_holidays_from_country
to hday_utils.py
neuralprophet/configure.py
Outdated
self.holiday_names = utils.get_holidays_from_country(self.country, df) | ||
self.holiday_names = self.get_holidays_from_country(self.country, df) | ||
|
||
def get_holidays_from_country(self, country: Union[str, Iterable[str], dict], df=None): |
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.
self
does not appear to be accessed in this method. I think we can safely move it to hdays_utils.py
.
…/neural_prophet into feature/1574-add-subdivisions
@MaiBe-ctrl Looks like you missed one utility function that was in |
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.
Feel free to merge once tests pass.
Model Benchmark
\n
Model training plots\n ## Model Training ### PeytonManning ![](https://asset.cml.dev/2674ed6eab06459d7904c5c6cdde50d02d1f0e43?cml=svg%2Bxml&cache-bypass=6eda5fb2-52dd-4d80-82d8-61388df583ce) ### YosemiteTemps ![](https://asset.cml.dev/80fd97c7b893476c67bf2022ad814bd7a52967b5?cml=svg%2Bxml&cache-bypass=67c5a318-a1a1-48d4-80b4-4170bc3081ac) ### AirPassengers ![](https://asset.cml.dev/85d0a8e9bd99f47bb431fadc7d312cb231fc02fe?cml=svg%2Bxml&cache-bypass=86afacde-0c16-4e29-af0d-87c876f2cd01) ### EnergyPriceDaily ![](https://asset.cml.dev/7ef5cc5693f75daaf6d7541b804c3711f76b8dd3?cml=svg%2Bxml&cache-bypass=c0ee71af-9aca-47fc-8139-2646d135a6af) \n |
🔬 Background
🔮 Key changes
📋 Review Checklist
Please make sure to follow our best practices in the Contributing guidelines.