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

[Bug-fix] Enable holidays when using global/glocal modeling #1100

Merged
merged 2 commits into from
Jan 11, 2023

Conversation

judussoari
Copy link
Collaborator

@judussoari judussoari commented Jan 7, 2023

🔬 Background

Fixes #1099
Adding country holidays with m.add_country_holidays() triggers an index error when the input df contains multiple time series (i.e. when glocal/global modeling is used).

🔮 Key changes

Resetting the index of the incoming time series in TimeDataset.make_events_features() resolves the problem.

📋 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.

@judussoari judussoari added this to the Release 0.5.1 milestone Jan 7, 2023
@judussoari judussoari self-assigned this Jan 7, 2023
@codecov-commenter
Copy link

codecov-commenter commented Jan 7, 2023

Codecov Report

Merging #1100 (982bbbc) into main (88c4711) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main    #1100   +/-   ##
=======================================
  Coverage   90.14%   90.14%           
=======================================
  Files          21       21           
  Lines        4798     4799    +1     
=======================================
+ Hits         4325     4326    +1     
  Misses        473      473           
Impacted Files Coverage Δ
neuralprophet/time_dataset.py 94.64% <100.00%> (+0.01%) ⬆️

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

@judussoari judussoari added the status: needs review PR needs to be reviewed by Reviewer(s) label Jan 7, 2023
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.

LGTM

@judussoari judussoari merged commit ebbe06a into main Jan 11, 2023
@judussoari judussoari deleted the global_holidays branch January 11, 2023 22:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: needs review PR needs to be reviewed by Reviewer(s)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[bug] Adding country holidays triggers Error when using global/glocal model
3 participants