TST: Added regression test #31538

ShaharNaveh · 2020-02-01T13:36:47Z

closes Strange error message when summing datetime64 and datetime.time column #10329
tests added / passed
passes black pandas
passes git diff upstream/master -u -- "*.py" | flake8 --diff
whatsnew entry

pep8speaks · 2020-02-01T13:36:51Z

Hello @MomIsBestFriend! 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-04-03 09:15:17 UTC

WillAyd

I think a more logical home might be pandas/tests/arrays/test_datetimes.py but @jbrockmendel might have more insight

pandas/tests/frame/test_timeseries.py

jbrockmendel · 2020-02-10T17:24:10Z

@MomIsBestFriend is moving this to tests.arithmetic something you're up for? (theres lots more that needs attention in that directory, so getting comfortable with the box_with_array pattern there would be worthwhile)

ShaharNaveh · 2020-02-11T20:43:35Z

@jbrockmendel I have to be honest here, I have tried to refactor the test case to use box_with_array, But unfortunately I couldn't make it, just seems odd to me, plus it's kinda lacking a documentation, so it's hard for me to do so, if you have patience I would love if you could guide me a bit on how to do so.

jbrockmendel · 2020-02-11T23:08:17Z

I have tried to refactor the test case to use box_with_array, But unfortunately I couldn't make it

I appreciate that you gave it a shot despite the lacking documentation.

The background: until relatively recently, we had separate arithmetic tests for Series/DataFrame/Index, and no systematic way of checking that these all behave the same (and in fact, they didnt). So we've been collecting arithmetic tests in tests.arithmetic and parametrizing over box_with_array (which is roughly just [DataFrame, Series, Index, pd.array]) to ensure that the behavior is internally consistent. As a result, we have a ton of tests there that are either duplicative or not fully parametrized.

The general pattern is to start with a test for just, say, Series:

def test_my_test():
     obj = pd.Series(...)
     other = ...
     expected = pd.Series(...)

     result = op(obj, other)
     tm.assert_series_equal(result, expected)

Parametrizing this over box_with_array would look like:

def test_my_test(box_with_array):
     obj = pd.Series(...)
     other = ...
     expected = pd.Series(...)  # <-- up to here, this can be copy/paste from the other version

     obj = tm.box_expected(obj, box_with_array)
     expected = tm.box_expected(expected, box_with_array)

     result = op(obj, other)
     tm.assert_equal(result, expected)

Does that help?

Note: for exposition this example is implicitly assuming that obj has a datetimelike dtype and that other is a scalar.

Co-Authored-By: William Ayd <[email protected]>

REF: pandas-dev#31538 (comment)

jbrockmendel · 2020-02-19T22:51:39Z

pandas/tests/arithmetic/test_datetime64.py

@@ -1056,6 +1056,22 @@ def test_dt64arr_add_sub_parr(
        )
        assert_invalid_addsub_type(dtarr, parr, msg)

+    def test_timestamp_and_time_dtype_raises(self, box_with_array):


"and" -> "addsub"

jbrockmendel · 2020-02-19T22:52:00Z

pandas/tests/arithmetic/test_datetime64.py

+        )
+
+        with pytest.raises(TypeError, match=msg):
+            obj1 - obj2


the reversed operation obj2 - obj1 should also raise right? How about the addition ops?

jbrockmendel · 2020-02-19T22:52:44Z

Can you add a time scalar to the other parametrization in test_dt64arr_add_sub_invalid?

This is looking really good, thanks.

REF: pandas-dev#31538 (comment)

jreback · 2020-03-03T03:22:03Z

lgtm. @jbrockmendel if any comments.

jbrockmendel · 2020-03-04T03:14:49Z

pandas/tests/arithmetic/test_datetime64.py

@@ -1056,6 +1057,33 @@ def test_dt64arr_add_sub_parr(
        )
        assert_invalid_addsub_type(dtarr, parr, msg)

+    def test_timestamp_addsub_time_dtype_raises(self, box_with_array):


"time_dtype" -> "time_objects"?
"timestamp" -> "dt64arr"

jbrockmendel · 2020-03-04T03:14:59Z

pandas/tests/arithmetic/test_datetime64.py

@@ -1056,6 +1057,33 @@ def test_dt64arr_add_sub_parr(
        )
        assert_invalid_addsub_type(dtarr, parr, msg)

+    def test_timestamp_addsub_time_dtype_raises(self, box_with_array):


tz_naive_fixture

jbrockmendel · 2020-03-04T03:15:40Z

pandas/tests/arithmetic/test_datetime64.py

+
+                # add
+                obj1 + obj2
+                obj2 + obj1


should we be testing that all four of these ops raise TypeError?

Yea this isn't very clear as is whether one or all of these raise a TypeError - can these be split into different contexts?

Or alternately you can parametrize add, radd, sub, rsub

Or alternately you can parametrize add, radd, sub, rsub

I can do that, is that's the preferred way?

dealer's choice. i tend not to parametrize over add/radd/sub/rsub since it repeats all the setup code for for what i see as little gain, but on a case-by-case basis if it improves clarity, go for it.

pandas/tests/arithmetic/test_datetime64.py

REF: pandas-dev#31538 (comment)

WillAyd · 2020-03-11T02:30:55Z

pandas/tests/arithmetic/test_datetime64.py

+
+                # add
+                obj1 + obj2
+                obj2 + obj1


Yea this isn't very clear as is whether one or all of these raise a TypeError - can these be split into different contexts?

REF: pandas-dev#31538 (comment)

pandas/tests/arithmetic/test_datetime64.py

REF: pandas-dev#31538 (comment)

jbrockmendel · 2020-03-18T19:35:01Z

pandas/tests/arithmetic/test_datetime64.py

@@ -1069,6 +1071,60 @@ def test_dt64arr_add_sub_parr(
        )
        assert_invalid_addsub_type(dtarr, parr, msg)

+    def test_dt64arr_addsub_time_object_raises(self, box_with_array, tz_naive_fixture):


nitpick: time_object -> time_objects. otherwise i think this is ready

REF: pandas-dev#31538 (comment)

jbrockmendel · 2020-03-26T01:48:04Z

Looks like tests are failing on some paths that go through numexpr

jreback · 2020-03-26T01:48:41Z

@MomIsBestFriend

can you merge master and see if you can get this to passing

jbrockmendel · 2020-03-30T18:47:46Z

LGTM

jreback · 2020-04-03T20:20:08Z

thanks @MomIsBestFriend

WillAyd added the Testing pandas testing functions or related to the test suite label Feb 1, 2020

WillAyd requested changes Feb 1, 2020

View reviewed changes

pandas/tests/frame/test_timeseries.py Outdated Show resolved Hide resolved

MomIsBestFriend and others added 4 commits February 14, 2020 15:43

TST: Added regression test

aa5dc3b

Flake8 issues

270dfee

Update pandas/tests/frame/test_timeseries.py

5691066

Co-Authored-By: William Ayd <[email protected]>

Moved the test case

2cf0756

REF: pandas-dev#31538 (comment)

ShaharNaveh force-pushed the TST-reg-datetime branch from c96a3ff to 33e24a3 Compare February 14, 2020 13:59

Refactored the test case to use 'box_with_array'

33e24a3

jbrockmendel reviewed Feb 19, 2020

View reviewed changes

MomIsBestFriend added 7 commits February 22, 2020 12:30

Changed test name

bc75e25

REF: pandas-dev#31538 (comment)

Merge remote-tracking branch 'upstream/master' into TST-reg-datetime

20ba781

Added the add operations

94cedb9

REF: pandas-dev#31538 (comment)

Added time scalar to "other"

1cafb66

REF: pandas-dev#31538 (comment)

Lint

a5608eb

Ignoring warnings, that we are not checking for this specific test case

fd04076

Merge remote-tracking branch 'upstream/master' into TST-reg-datetime

1d17645

jreback added this to the 1.1 milestone Mar 3, 2020

jreback added the Timedelta Timedelta data type label Mar 3, 2020

WillAyd approved these changes Mar 4, 2020

View reviewed changes

jbrockmendel reviewed Mar 4, 2020

View reviewed changes

pandas/tests/arithmetic/test_datetime64.py Show resolved Hide resolved

MomIsBestFriend added 4 commits March 5, 2020 12:34

Merge remote-tracking branch 'upstream/master' into TST-reg-datetime

c454822

Renamed test case

faf90e1

REF: pandas-dev#31538 (comment)

Added comment pointing to the appropiate issue

5cdd1d1

REF: pandas-dev#31538 (comment)

Added tz_naive_fixture

6fef7f8

REF: pandas-dev#31538 (comment)

WillAyd requested changes Mar 11, 2020

View reviewed changes

MomIsBestFriend added 3 commits March 13, 2020 12:29

Putting each expression in its own pytest.raises

799ae78

REF: pandas-dev#31538 (comment)

Fixed tests to pass

54ea5a0

Merge remote-tracking branch 'upstream/master' into TST-reg-datetime

808599b

jbrockmendel reviewed Mar 13, 2020

View reviewed changes

pandas/tests/arithmetic/test_datetime64.py Outdated Show resolved Hide resolved

jbrockmendel reviewed Mar 13, 2020

View reviewed changes

pandas/tests/arithmetic/test_datetime64.py Show resolved Hide resolved

MomIsBestFriend added 2 commits March 13, 2020 20:34

Removed unneeded comments

0db9073

REF: pandas-dev#31538 (comment)

Merge remote-tracking branch 'upstream/master' into TST-reg-datetime

d62cad3

jbrockmendel reviewed Mar 18, 2020

View reviewed changes

MomIsBestFriend added 4 commits March 18, 2020 22:04

Renamed test case

1449d8c

REF: pandas-dev#31538 (comment)

Merge remote-tracking branch 'upstream/master' into TST-reg-datetime

c5dc76c

Merge remote-tracking branch 'upstream/master' into TST-reg-datetime

6d2e72f

Merge remote-tracking branch 'upstream/master' into TST-reg-datetime

5742c46

MomIsBestFriend added 4 commits March 26, 2020 04:36

Merge remote-tracking branch 'upstream/master' into TST-reg-datetime

951074f

Merge remote-tracking branch 'upstream/master' into TST-reg-datetime

fbce136

Fixed error message

5954709

Fixed error message

f51559f

Merge remote-tracking branch 'upstream/master' into TST-reg-datetime

e183a23

jreback merged commit a44ac34 into pandas-dev:master Apr 3, 2020

ShaharNaveh deleted the TST-reg-datetime branch April 6, 2020 08:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

TST: Added regression test #31538

TST: Added regression test #31538

ShaharNaveh commented Feb 1, 2020

pep8speaks commented Feb 1, 2020 •

edited

Loading

WillAyd left a comment

jbrockmendel commented Feb 10, 2020

ShaharNaveh commented Feb 11, 2020 •

edited

Loading

jbrockmendel commented Feb 11, 2020

jbrockmendel Feb 19, 2020

jbrockmendel Feb 19, 2020

jbrockmendel commented Feb 19, 2020

jreback commented Mar 3, 2020

jbrockmendel Mar 4, 2020

jbrockmendel Mar 4, 2020

jbrockmendel Mar 4, 2020

WillAyd Mar 11, 2020

WillAyd Mar 11, 2020

ShaharNaveh Mar 13, 2020

jbrockmendel Mar 13, 2020

WillAyd Mar 11, 2020

jbrockmendel Mar 18, 2020

jbrockmendel commented Mar 26, 2020

jreback commented Mar 26, 2020

jbrockmendel commented Mar 30, 2020

jreback commented Apr 3, 2020

TST: Added regression test #31538

TST: Added regression test #31538

Conversation

ShaharNaveh commented Feb 1, 2020

pep8speaks commented Feb 1, 2020 • edited Loading

Comment last updated at 2020-04-03 09:15:17 UTC

WillAyd left a comment

Choose a reason for hiding this comment

jbrockmendel commented Feb 10, 2020

ShaharNaveh commented Feb 11, 2020 • edited Loading

jbrockmendel commented Feb 11, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbrockmendel commented Feb 19, 2020

jreback commented Mar 3, 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

jbrockmendel commented Mar 26, 2020

jreback commented Mar 26, 2020

jbrockmendel commented Mar 30, 2020

jreback commented Apr 3, 2020

pep8speaks commented Feb 1, 2020 •

edited

Loading

ShaharNaveh commented Feb 11, 2020 •

edited

Loading