Skip to content

Fix bug in calculation of holidays #31416

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.

Sign up for GitHub

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

Merged
merged 5 commits into from
Feb 9, 2020
+20 −9
Merged

Fix bug in calculation of holidays #31416

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
29eb7a4
Fix bug in calculation of holidays
Feb 2, 2020
be850b7
Address review comments
Feb 5, 2020
f0f0f51
Fix type check in test
Feb 7, 2020
b50995b
Fix lint
dhirschfeld Feb 7, 2020
b02918a
Use appropriate assert function
dhirschfeld Feb 7, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions doc/source/whatsnew/v1.1.0.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -223,6 +223,8 @@ Other
- Appending a dictionary to a :class:`DataFrame` without passing ``ignore_index=True`` will raise ``TypeError: Can only append a dict if ignore_index=True``
instead of ``TypeError: Can only append a Series if ignore_index=True or if the Series has a name`` (:issue:`30871`)
- Set operations on an object-dtype :class:`Index` now always return object-dtype results (:issue:`31401`)
- Bug in :meth:`AbstractHolidayCalendar.holidays` when no rules were defined (:issue:`31415`)
Copy link
Contributor

Choose a reason for hiding this comment

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

We ought to limit notes in the Other section if at all possible if someone wants to try to classify these better

cc @pandas-dev/pandas-core

-

.. ---------------------------------------------------------------------------

Expand Down
11 changes: 11 additions & 0 deletions pandas/tests/tseries/holiday/test_calendar.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,3 +98,14 @@ class testCalendar(AbstractHolidayCalendar):
Sat_before_Labor_Day_2031 = to_datetime("2031-08-30")
next_working_day = Sat_before_Labor_Day_2031 + 0 * workDay
assert next_working_day == to_datetime("2031-09-02")


def test_no_holidays_calendar():
# Test for issue #31415

class NoHolidaysCalendar(AbstractHolidayCalendar):
Copy link
Contributor

Choose a reason for hiding this comment

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

add the issue number here as a comment

pass

cal = NoHolidaysCalendar()
holidays = cal.holidays(Timestamp("01-Jan-2020"), Timestamp("01-Jan-2021"))
assert holidays.empty
Copy link
Contributor

Choose a reason for hiding this comment

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

can you use tm.assert_series_equal here and construct an empty Series with a DTI?

15 changes: 6 additions & 9 deletions pandas/tseries/holiday.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@

from pandas.errors import PerformanceWarning

from pandas import DateOffset, Series, Timestamp, date_range
from pandas import DateOffset, DatetimeIndex, Series, Timestamp, concat, date_range

from pandas.tseries.offsets import Day, Easter

Expand Down Expand Up @@ -406,17 +406,14 @@ def holidays(self, start=None, end=None, return_name=False):
start = Timestamp(start)
end = Timestamp(end)

holidays = None
# If we don't have a cache or the dates are outside the prior cache, we
# get them again
if self._cache is None or start < self._cache[0] or end > self._cache[1]:
for rule in self.rules:
rule_holidays = rule.dates(start, end, return_name=True)

if holidays is None:
holidays = rule_holidays
else:
holidays = holidays.append(rule_holidays)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This implementation also has the benefit of doing a single concat outside the loop rather than an O(N²) append on each iteration.

holidays = [rule.dates(start, end, return_name=True) for rule in self.rules]
if holidays:
holidays = concat(holidays)
else:
holidays = Series(index=DatetimeIndex([]), dtype=object)
Copy link
Member

Choose a reason for hiding this comment

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

Doesn't holidays typically return a DatetimeIndex ? Should maintain the same type even when empty

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Internally, the holidays are constructed with return_name=True which returns a Series with the holiday names as values:

In [7]: USFederalHolidayCalendar().holidays('01-Jan-2020', '01-Apr-2020', return_name=True)
Out[7]: 
2020-01-01                 New Years Day
2020-01-20    Martin Luther King Jr. Day
2020-02-17                Presidents Day
dtype: object

Only if return_name=False are the values striped at the end of the method - behaviour which remains unchanged from master:

pandas/pandas/tseries/holiday.py

Lines 426 to 429 in ba08390

if return_name:
return holidays
else:
return holidays.index


self._cache = (start, end, holidays.sort_index())

Expand Down