Skip to content
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

[refactor] Update python-holidays integration #1176

Merged
merged 3 commits into from
Feb 28, 2023

Conversation

arkid15r
Copy link
Contributor

🔬 Background

Resolves #295

🔮 Key changes

  • Bump python-holidays version to 0.20.
  • Migrate to python-holidays as a single source of country holidays.

📋 Review Checklist

  • I have performed a self-review of my own code.
  • I have commented my code, added docstrings and data types to function definitions.
  • I have added pytests to check whether my feature / fix works.

Please make sure to follow our best practices in the Contributing guidelines.

Bump python-holidays version to 0.20.
Migrate to python-holidays as a single source of country holidays.
@codecov-commenter
Copy link

codecov-commenter commented Feb 28, 2023

Codecov Report

Merging #1176 (e029657) into main (5770171) will decrease coverage by 0.71%.
The diff coverage is 100.00%.

📣 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

@@            Coverage Diff             @@
##             main    #1176      +/-   ##
==========================================
- Coverage   90.20%   89.49%   -0.71%     
==========================================
  Files          36       35       -1     
  Lines        5145     4770     -375     
==========================================
- Hits         4641     4269     -372     
+ Misses        504      501       -3     
Impacted Files Coverage Δ
neuralprophet/hdays_utils.py 100.00% <100.00%> (+16.66%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

Copy link
Owner

@ourownstory ourownstory left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@arkid15r Wonderful, thank you so much for upgrading holidays with the needed countries and for updating our use of holidays, including tests.
Much appreciated!

@arkid15r
Copy link
Contributor Author

@arkid15r Wonderful, thank you so much for upgrading holidays with the needed countries and for updating our use of holidays, including tests. Much appreciated!

Hi @ourownstory,
thanks for the quick reply!

I'm not sure whether it's safe to merge the PR? Those failing metrics and windows tests jobs look a bit concerning.

Copy link
Collaborator

@noxan noxan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks for the awesome contribution @arkid15r 👏

Edit: oh didn't see your review @ourownstory :)

@noxan
Copy link
Collaborator

noxan commented Feb 28, 2023

@arkid15r Those are (sadly) known issues: windows tests failing are tracked in #991 and the metrics do not work on forks #1113 - so I'll go ahead and merge it

@noxan noxan changed the title Update python-holidays integration [refactor] Update python-holidays integration Feb 28, 2023
@noxan noxan merged commit 7ed2d9c into ourownstory:main Feb 28, 2023
@arkid15r arkid15r deleted the update-python-holidays-integration branch February 28, 2023 01:20
@github-actions
Copy link

Model Benchmark

Benchmark Metric main current diff
YosemiteTemps MAE_val 1.72949 1.72948 -0.0%
YosemiteTemps RMSE_val 2.27386 2.27386 -0.0%
YosemiteTemps Loss_val 0.00096 0.00096 -0.0%
YosemiteTemps MAE 1.45189 1.45189 -0.0%
YosemiteTemps RMSE 2.16631 2.16631 -0.0%
YosemiteTemps Loss 0.00066 0.00066 0.0%
YosemiteTemps time 151.017 167.6 10.98%
AirPassengers MAE_val 15.4077 15.4077 0.0%
AirPassengers RMSE_val 19.5099 19.5099 0.0%
AirPassengers Loss_val 0.00196 0.00196 0.0%
AirPassengers MAE 9.86947 9.86947 -0.0%
AirPassengers RMSE 11.7222 11.7222 -0.0%
AirPassengers Loss 0.00057 0.00057 -0.0%
AirPassengers time 5.6505 7.41 31.14%
PeytonManning MAE_val 0.64636 0.64636 -0.0%
PeytonManning RMSE_val 0.79276 0.79276 -0.0%
PeytonManning Loss_val 0.01494 0.01494 -0.0%
PeytonManning MAE 0.42701 0.42701 0.0%
PeytonManning RMSE 0.57032 0.57032 -0.0%
PeytonManning Loss 0.00635 0.00635 0.0%
PeytonManning time 15.509 21.1 36.05%
Model training plots

Model Training

PeytonManning

YosemiteTemps

AirPassengers

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Holidays: Remove manually defined holidays once supported by python-holidays
4 participants