BUG: fix datetimes.should_cache() error for deque (GH 29403) #29846

nathalier · 2019-11-26T00:44:22Z

itertools.islice() should be used to get slice of a deque. itertools.islice() also can be used (and is efficient) for other collections. So unique(arg[:check_count]) was replaced with set(islice(arg, check_count)) which is also much faster on test data.

closes pd.to_datetime should_cache errors with deque #29403
tests added / passed
passes black pandas
passes git diff upstream/master -u -- "*.py" | flake8 --diff
whatsnew entry

pep8speaks · 2019-11-26T00:44:26Z

Hello @nathalier! 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 2019-12-14 16:01:44 UTC

mroeschke · 2019-11-26T04:49:31Z

pandas/tests/indexes/datetimes/test_tools.py

+)
+def test_no_slicing_errors_in_should_cache(listlike):
+    # GH 29403
+    assert tools.should_cache(listlike) is True


Instead, could you use the example in the original issues as the test? #29403 (comment)

@mroeschke For this particular bug the example from the original issue could easily stop being indicative if the empirically chosen constants for caching are changed. Let's say if it increased from 50 to 55 or 70 or 100 then the original issue example won't run this code pass, whereas these tests will show that something changed as they would fail. Also they are faster than the original example. And as we definitely know what was the reason of the failure I think it's quite reasonable to use small and fast tests for should_cache() instead.
The original example can be added to ensure the deque works for datetimes. Would be grateful if you could advise to which test file I should add that. Can it be pandas/tests/arrays/test_datetimes.py ?

@mroeschke For this particular bug the example from the original issue could easily stop being indicative if the empirically chosen constants for caching are changed. Let's say if it increased from 50 to 55 or 70 or 100 then the original issue example won't run this code pass, whereas these tests will show that something changed as they would fail. Also they are faster than the original example. And as we definitely know what was the reason of the failure I think it's quite reasonable to use small and fast tests for should_cache() instead.
The original example can be added to ensure deque works for timeseries. Would be grateful if you could advise to which test file I should add that. Can it be pandas/tests/arrays/test_datetimes.py ?

Good point about the threshold; we can keep the test you have and add the example in the original issue. I think the pandas/tests/arrays/test_datetimes.py is a good place for this test.

the current location is good for this test

@mroeschke I've found a very relevant test test_to_datetime_cache() where I included deque in the list of constructors. Actually, with this test, tests I added before for the bug are not much needed any more. What do you think - should I drop them (test_no_slicing_errors_in_should_cache() and test_to_datetime_from_deque()) now?

I'm okay keeping both tests. These tests run relatively quickly.

hmm, these do look quite duplicative, though these are hard-coded to the results which is fine.

can you co-located these tests near the above one.

jbrockmendel · 2019-12-04T02:28:39Z

pls run black pandas/tests/indexes/datetimes/test_tools.py and isort pandas/tests/indexes/datetimes/test_tools.py

jreback

can you add a whatsnew note (bug fixes / datetimes), otherwise lgtm. ping on green.

nathalier · 2019-12-07T21:51:22Z

pls run black pandas/tests/indexes/datetimes/test_tools.py and isort pandas/tests/indexes/datetimes/test_tools.py

@jbrockmendel Thanks a lot for help!

nathalier · 2019-12-07T21:54:17Z

@jreback Now it's green, but possibly some of added tests are redundant.

mroeschke · 2019-12-08T00:08:30Z

doc/source/whatsnew/v1.0.0.rst

@@ -679,6 +679,8 @@ Datetimelike
 - Bug in :meth:`Series.var` failing to raise ``TypeError`` when called with ``timedelta64[ns]`` dtype (:issue:`28289`)
 - Bug in :meth:`DatetimeIndex.strftime` and :meth:`Series.dt.strftime` where ``NaT`` was converted to the string ``'NaT'`` instead of ``np.nan`` (:issue:`29578`)
 - Bug in :attr:`Timestamp.resolution` being a property instead of a class attribute (:issue:`29910`)
+- Bug in :func:`pandas.core.tools.datetimes.should_cache` raising ``TypeError`` when trying to get a slice of a deque (:issue:`29403`)


Could you reword to make more user facing? i.e. to_datetime failing for deques when using the cache

Thanks for the hint. Reworded.

jreback

minor comments, lgtm. ping on green.

jreback · 2019-12-10T14:44:09Z

doc/source/whatsnew/v1.0.0.rst

@@ -679,6 +679,7 @@ Datetimelike
 - Bug in :meth:`Series.var` failing to raise ``TypeError`` when called with ``timedelta64[ns]`` dtype (:issue:`28289`)
 - Bug in :meth:`DatetimeIndex.strftime` and :meth:`Series.dt.strftime` where ``NaT`` was converted to the string ``'NaT'`` instead of ``np.nan`` (:issue:`29578`)
 - Bug in :attr:`Timestamp.resolution` being a property instead of a class attribute (:issue:`29910`)
+- Bug in :func:`pandas.core.tools.datetimes.should_cache` failing for `deques` with ``TypeError`` when using the cache (:issue:`29403`)


can you change this to to_datetime which is the user visibile function
when using cache=True (the default)

@jreback Tests moved, whatsnew reworded. All tests have passed.

jreback · 2019-12-10T14:46:01Z

pandas/tests/indexes/datetimes/test_tools.py

+)
+def test_no_slicing_errors_in_should_cache(listlike):
+    # GH 29403
+    assert tools.should_cache(listlike) is True


hmm, these do look quite duplicative, though these are hard-coded to the results which is fine.

can you co-located these tests near the above one.

itertools.islice() should be used to get slice of a deque. itertools.islice() also can be used (and is efficient) for other collections. So unique(arg[:check_count]) was replaced with set(islice(arg, check_count))

jreback · 2019-12-15T22:12:02Z

thanks @nathalier

…dev#29846)

nathalier force-pushed the gh-29403 branch from 24a7d06 to 3a0ac42 Compare November 26, 2019 00:53

mroeschke reviewed Nov 26, 2019

View reviewed changes

mroeschke added the Datetime Datetime data dtype label Nov 26, 2019

jreback added this to the 1.0 milestone Dec 4, 2019

jreback added the Bug label Dec 4, 2019

jreback requested changes Dec 4, 2019

View reviewed changes

nathalier force-pushed the gh-29403 branch 2 times, most recently from b0a9658 to ca98e77 Compare December 7, 2019 17:43

mroeschke reviewed Dec 8, 2019

View reviewed changes

nathalier force-pushed the gh-29403 branch from bd54627 to b2ca829 Compare December 9, 2019 01:52

nathalier requested a review from jreback December 10, 2019 14:41

jreback requested changes Dec 10, 2019

View reviewed changes

nathalier added 2 commits December 14, 2019 15:58

BUG: fix datetimes.should_cache() error for deque (GH 29403)

e269670

itertools.islice() should be used to get slice of a deque. itertools.islice() also can be used (and is efficient) for other collections. So unique(arg[:check_count]) was replaced with set(islice(arg, check_count))

add whatsnew for pandas-devGH-29403

9389cee

nathalier force-pushed the gh-29403 branch from b2ca829 to 9389cee Compare December 14, 2019 16:01

mroeschke approved these changes Dec 14, 2019

View reviewed changes

jreback approved these changes Dec 15, 2019

View reviewed changes

jreback merged commit 37dfcc1 into pandas-dev:master Dec 15, 2019

proost pushed a commit to proost/pandas that referenced this pull request Dec 19, 2019

BUG: fix datetimes.should_cache() error for deque (GH 29403) (pandas-…

b7a1dc5

…dev#29846)

proost pushed a commit to proost/pandas that referenced this pull request Dec 19, 2019

BUG: fix datetimes.should_cache() error for deque (GH 29403) (pandas-…

2bcc3f6

…dev#29846)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

BUG: fix datetimes.should_cache() error for deque (GH 29403) #29846

BUG: fix datetimes.should_cache() error for deque (GH 29403) #29846

nathalier commented Nov 26, 2019 •

edited

Loading

pep8speaks commented Nov 26, 2019 •

edited

Loading

mroeschke Nov 26, 2019 •

edited

Loading

nathalier Nov 30, 2019

nathalier Nov 30, 2019

mroeschke Nov 30, 2019

jreback Nov 30, 2019

nathalier Dec 7, 2019

mroeschke Dec 8, 2019

jreback Dec 10, 2019

jbrockmendel commented Dec 4, 2019

jreback left a comment

nathalier commented Dec 7, 2019

nathalier commented Dec 7, 2019

mroeschke Dec 8, 2019

nathalier Dec 9, 2019

jreback left a comment

jreback Dec 10, 2019

nathalier Dec 14, 2019

jreback Dec 10, 2019

jreback commented Dec 15, 2019

BUG: fix datetimes.should_cache() error for deque (GH 29403) #29846

BUG: fix datetimes.should_cache() error for deque (GH 29403) #29846

Conversation

nathalier commented Nov 26, 2019 • edited Loading

pep8speaks commented Nov 26, 2019 • edited Loading

Comment last updated at 2019-12-14 16:01:44 UTC

mroeschke Nov 26, 2019 • edited Loading

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

jbrockmendel commented Dec 4, 2019

jreback left a comment

Choose a reason for hiding this comment

nathalier commented Dec 7, 2019

nathalier commented Dec 7, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jreback left a 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 Dec 15, 2019

nathalier commented Nov 26, 2019 •

edited

Loading

pep8speaks commented Nov 26, 2019 •

edited

Loading

mroeschke Nov 26, 2019 •

edited

Loading