Fix bug in calculation of holidays #31416

dhirschfeld · 2020-01-29T12:19:32Z

Closes #31415

pep8speaks · 2020-01-29T12:19:38Z

Hello @dhirschfeld! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2020-02-07 04:22:19 UTC

jbrockmendel · 2020-01-29T16:47:34Z

Can you add tests for the behavior this fixes

dhirschfeld · 2020-01-30T13:52:01Z

pandas/tseries/holiday.py

-                if holidays is None:
-                    holidays = rule_holidays
-                else:
-                    holidays = holidays.append(rule_holidays)


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

dhirschfeld · 2020-01-31T06:29:56Z

CI is passing! 🎉

WillAyd · 2020-01-31T14:36:30Z

doc/source/whatsnew/v1.0.1.rst

@@ -16,6 +16,8 @@ including other versions of pandas.
 Bug fixes
 ~~~~~~~~~

+- Bug in :meth:`AbstractHolidayCalendar.holidays` when no rules were defined (:issue:`31415`)


Did this change between 0.25.X and 1.0.0? If not should target 1.1.0 instead of 1.0.1

WillAyd · 2020-01-31T14:40:40Z

doc/source/whatsnew/v1.0.1.rst

@@ -16,6 +16,8 @@ including other versions of pandas.
 Bug fixes
 ~~~~~~~~~

+- Bug in :meth:`AbstractHolidayCalendar.holidays` when no rules were defined (:issue:`31415`)


Is this bug new between 0.25.x and 1.0.0? If not should target 1.1.0 instead of 1.0.1

It looks like this bug has been present for the last 6 years. I don't think the AbstractHolidayCalendar functionality gets all that much use and this is a particular corner case.

Given that this PR fixes a bug and changes no apis or behaviours, other than returning a sensible value instead of throwing an unintended exception, I think this should get in the soonest possible release which IIUC would be 1.0.1.

I'm not sure why when the bug was introduced should have any bearing on when it can be fixed. I think it's fine to fix bugs which don't change any apis or behaviours in patch releases - I think this is exactly what patch releases are for!

@dhirschfeld pls move to 1.1 whatsnew

WillAyd · 2020-01-31T14:43:17Z

pandas/tseries/holiday.py

+            if holidays:
+                holidays = concat(holidays)
+            else:
+                holidays = Series(index=DatetimeIndex([]), dtype=object)


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

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

jreback · 2020-02-01T14:59:07Z

doc/source/whatsnew/v1.0.1.rst

@@ -16,6 +16,8 @@ including other versions of pandas.
 Bug fixes
 ~~~~~~~~~

+- Bug in :meth:`AbstractHolidayCalendar.holidays` when no rules were defined (:issue:`31415`)


@dhirschfeld pls move to 1.1 whatsnew

jreback · 2020-02-01T14:59:23Z

pandas/tests/tseries/holiday/test_calendar.py

+
+
+def test_no_holidays_calendar():
+    class NoHolidaysCalendar(AbstractHolidayCalendar):


add the issue number here as a comment

jreback · 2020-02-01T15:00:04Z

pandas/tseries/holiday.py

        # 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)
+                holidays.append(rule.dates(start, end, return_name=True))


can you re-write as a list-comprehension

jreback · 2020-02-02T17:15:24Z

code change looks fine. I think you need to isort the files you change. btw we have a pre-commit hook for this (pip install precommit turns it on).

merge master and ping on green.

dhirschfeld · 2020-02-03T00:34:37Z

I rebased on master but I think I'm now getting an unrelated failure bout libindex:

black --version
black, version 19.10b0
Checking black formatting
All done! ✨ 🍰 ✨
941 files would be left unchanged.
Checking black formatting DONE
flake8 --version
3.7.9 (flake8-comprehensions: 3.2.2, mccabe: 0.6.1, pycodestyle: 2.5.0, pyflakes: 2.1.1) CPython 3.7.6 on Linux
Linting .py code
##[error]./pandas/core/series.py:1025:9:F821:undefined name 'libindex'
##[error]./pandas/core/series.py:1108:17:F821:undefined name 'libindex'
Linting .py code DONE

dhirschfeld · 2020-02-03T00:37:31Z

Possibly related to #31506?

Closes pandas-dev#31415

dhirschfeld · 2020-02-04T11:22:10Z

ping @jreback - rebased on (the latest) master and the tests are passing! 🎉

jreback · 2020-02-05T01:47:58Z

pandas/tests/tseries/holiday/test_calendar.py

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


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

dhirschfeld · 2020-02-07T05:13:06Z

ping @jreback!

jreback · 2020-02-09T20:54:42Z

thanks @dhirschfeld very nice

jreback · 2020-02-09T20:55:45Z

doc/source/whatsnew/v1.1.0.rst

@@ -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`)


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

dhirschfeld force-pushed the patch-4 branch 2 times, most recently from a29e232 to 9ad40d5 Compare January 30, 2020 13:47

dhirschfeld commented Jan 30, 2020

View reviewed changes

dhirschfeld requested a review from jbrockmendel January 30, 2020 13:52

dhirschfeld force-pushed the patch-4 branch 2 times, most recently from 96bcd89 to 10b2759 Compare January 31, 2020 06:01

WillAyd requested changes Jan 31, 2020

View reviewed changes

WillAyd added the Datetime Datetime data dtype label Jan 31, 2020

jreback requested changes Feb 1, 2020

View reviewed changes

jreback added the Frequency DateOffsets label Feb 1, 2020

dhirschfeld force-pushed the patch-4 branch 4 times, most recently from 0e62415 to 3d84f8b Compare February 3, 2020 00:11

dhirschfeld closed this Feb 4, 2020

dhirschfeld reopened this Feb 4, 2020

Fix bug in calculation of holidays

29eb7a4

Closes pandas-dev#31415

dhirschfeld force-pushed the patch-4 branch from 3d84f8b to 29eb7a4 Compare February 4, 2020 10:50

jreback requested changes Feb 5, 2020

View reviewed changes

David Hirschfeld and others added 4 commits February 5, 2020 12:32

Address review comments

be850b7

Fix type check in test

f0f0f51

Fix lint

b50995b

Use appropriate assert function

b02918a

jreback added this to the 1.1 milestone Feb 9, 2020

jreback approved these changes Feb 9, 2020

View reviewed changes

jreback merged commit c4e8eb3 into pandas-dev:master Feb 9, 2020

jreback reviewed Feb 9, 2020

View reviewed changes

dhirschfeld deleted the patch-4 branch February 9, 2020 23:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix bug in calculation of holidays #31416

Fix bug in calculation of holidays #31416

dhirschfeld commented Jan 29, 2020 •

edited

Loading

pep8speaks commented Jan 29, 2020 •

edited

Loading

jbrockmendel commented Jan 29, 2020

dhirschfeld Jan 30, 2020

dhirschfeld commented Jan 31, 2020

WillAyd Jan 31, 2020

WillAyd Jan 31, 2020

dhirschfeld Feb 1, 2020

jreback Feb 1, 2020

WillAyd Jan 31, 2020

dhirschfeld Feb 1, 2020

jreback Feb 1, 2020

jreback Feb 1, 2020

jreback Feb 1, 2020

jreback commented Feb 2, 2020

dhirschfeld commented Feb 3, 2020

dhirschfeld commented Feb 3, 2020

dhirschfeld commented Feb 4, 2020

jreback Feb 5, 2020

dhirschfeld commented Feb 7, 2020

jreback commented Feb 9, 2020

jreback Feb 9, 2020



		def test_no_holidays_calendar():
		class NoHolidaysCalendar(AbstractHolidayCalendar):

Fix bug in calculation of holidays #31416

Fix bug in calculation of holidays #31416

Conversation

dhirschfeld commented Jan 29, 2020 • edited Loading

pep8speaks commented Jan 29, 2020 • edited Loading

Comment last updated at 2020-02-07 04:22:19 UTC

jbrockmendel commented Jan 29, 2020

Choose a reason for hiding this comment

dhirschfeld commented Jan 31, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jreback commented Feb 2, 2020

dhirschfeld commented Feb 3, 2020

dhirschfeld commented Feb 3, 2020

dhirschfeld commented Feb 4, 2020

Choose a reason for hiding this comment

dhirschfeld commented Feb 7, 2020

jreback commented Feb 9, 2020

Choose a reason for hiding this comment

dhirschfeld commented Jan 29, 2020 •

edited

Loading

pep8speaks commented Jan 29, 2020 •

edited

Loading