separate numeric tests so we can isolate division by zero #19336

jbrockmendel · 2018-01-22T00:21:09Z

The upcoming fix(es) for #19322 are going to involve parametrizing a bunch of variants of division by zero. This separates out the existing test cases into method-specific tests, some of which will be parametrized in upcoming PRs.

This PR does not change the aggregate contents of the tests.

jreback · 2018-01-22T11:33:57Z

pandas/tests/series/test_operators.py

@@ -595,77 +595,103 @@ def test_divide_decimal(self):

        assert_series_equal(expected, s)

-    def test_div(self):
+    def test_i8_ser_div_i8_ser(self):


could parametrize on the .astype(np.float32 & np.float64), it doesn't make a real difference, but could check i guess

Your comments are all correct about parametrization and redundancy. This PR got split off of a branch that tried to address all of it at once and got huge because many cases currently fail. The idea is to split it first in a no-actual-changes-so-easy-to-review way, then apply the fixes one at a time.

jreback · 2018-01-22T11:35:50Z

pandas/tests/series/test_operators.py

-            result = p['first'] / p['second']
-            expected = Series(p['first'].values / p['second'].values)
-            assert_series_equal(result, expected)
+    def test_f8_ser_div_f8_ser(self):


same as above, maybe make a fixture for float32/float64 (could put at in pandas/tests/conftest.py) and for ints/uints as well. I think there is an issue about doing this.

jreback · 2018-01-22T11:36:22Z

pandas/tests/series/test_operators.py

-            # GH 7785
-            p = DataFrame({'first': (1, 0), 'second': (-0.01, -0.02)})
-            expected = Series([-0.01, -np.inf])
+    def test_ser_div_ser_name_propagation(self):


why is this a separate test and not in the div test?

Not really sure what the original motivation was here. TBH I was just guessing that the point was name propagation, since it wasn't clear what else it could be for.

jreback · 2018-01-22T11:37:03Z

pandas/tests/series/test_operators.py


-            result = p['second'] / p['first']
-            assert_series_equal(result, expected)
+    def test_int_div_pyint_zero(self):


parametrize over 0 and numpy int 0 scalars (and maybe uints for that matter)

Yep, I ended up putting a func in pd.util.testing to generate variants of zeros. Holding off on it for now because many variants fail ATM.

jreback · 2018-01-22T11:37:40Z

pandas/tests/series/test_operators.py

-            expected = pd.Series([0.] * 5)
-            result = zero_array / pd.Series(data)
-            assert_series_equal(result, expected)
+    def test_rdiv_zero_compat(self):


parametrie over 0 with various int/uint dtypes

jreback · 2018-01-22T11:37:56Z

pandas/tests/series/test_operators.py

+        result = pd.Series(zero_array) / pd.Series(data)
+        assert_series_equal(result, expected)
+
+    def test_div_zero(self):


duplicate of test above

jreback · 2018-01-22T11:38:06Z

pandas/tests/series/test_operators.py

+        result = ser / 0
+        assert_series_equal(result, expected)
+
+    def test_rdiv_zero(self):


parametrize

jreback · 2018-01-22T11:38:33Z

pandas/tests/series/test_operators.py

+        result = 0 / ser
+        assert_series_equal(result, expected)
+
+    def test_floordiv_div(self):


same. see the pattern :>

need to create a zero fixture

jreback · 2018-01-22T11:38:51Z

pandas/tests/series/test_operators.py

@@ -1574,28 +1600,28 @@ def test_dt64_series_add_intlike(self, tz):


 class TestSeriesOperators(TestData):
-    def test_op_method(self):


can you add a comment to describe what tests this class does

jreback · 2018-01-22T11:39:57Z

pandas/tests/series/test_operators.py

-                result = op(series, other)
-                expected = alt(series, other)
+            if op == 'div':
+                alt = operator.truediv


instead of making check an inner function, just add another layer of paramaterization (for self.ts * 2 and self.ts[::2])

How do you put self.ts in the parametrization? Unless you mean just replacing it with the evaluated series itself which I'd prefer anyway.

I think this is incorrect anyway since op == 'div' should never be True; this is probably supposed to be if opname == div; and in fact that check doesn't matter because we're excluding "div" on PY3 anyway...

you would put callables, e.g.
```"ts", [lambda x: x * 2, lambda x: x[::2]]and just injectself.ts``

e.g.

def test_.....(ts): ts = ts(self.ts)

This sounds unnecessarily convoluted. Dummy lambdas and parametrization obfuscate what is being tested (which is already not-immediately-obvious, as you mentioned earlier).

this is standard pytest usage. and much much more clear than the current.

i'd like this changed

jbrockmendel · 2018-01-22T18:16:41Z

Just implemented most of the requests. The parametrization of zeros is being put off for now because specifying all of the existing failure modes is a task unto itself.

jreback · 2018-01-23T00:23:54Z

pandas/tests/series/test_operators.py

@@ -595,77 +595,85 @@ def test_divide_decimal(self):

        assert_series_equal(expected, s)

-    def test_div(self):
+    @pytest.mark.parametrize('dtype2', [np.int64, np.int32, np.int16, np.int8,


wouldn't this be better as a fixture

Maybe eventually. ATM it is only used once so there's no upside.

as a style note, generally I would write this like

@pytest.mark.parameterize( 'dtype2', [ np.int64, ......])

as it makes it shorter and more readable

i'd like this changed

codecov · 2018-01-23T01:28:08Z

Codecov Report

Merging #19336 into master will increase coverage by 0.02%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master   #19336      +/-   ##
==========================================
+ Coverage   91.67%   91.69%   +0.02%     
==========================================
  Files         148      148              
  Lines       48553    48553              
==========================================
+ Hits        44513    44523      +10     
+ Misses       4040     4030      -10

Flag	Coverage Δ
#multiple	`90.06% <ø> (+0.02%)`	⬆️
#single	`41.72% <ø> (ø)`	⬆️

Impacted Files	Coverage Δ
pandas/util/testing.py	`83.64% <0%> (-0.21%)`	⬇️
pandas/plotting/_converter.py	`66.95% <0%> (+1.73%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 35812ea...4b78542. Read the comment docs.

jreback · 2018-01-23T11:42:24Z

pandas/tests/series/test_operators.py

-                result = op(series, other)
-                expected = alt(series, other)
+            if op == 'div':
+                alt = operator.truediv


this is standard pytest usage. and much much more clear than the current.

jreback · 2018-01-24T01:26:03Z

pandas/tests/series/test_operators.py

@@ -595,77 +595,85 @@ def test_divide_decimal(self):

        assert_series_equal(expected, s)

-    def test_div(self):
+    @pytest.mark.parametrize('dtype2', [np.int64, np.int32, np.int16, np.int8,


as a style note, generally I would write this like

@pytest.mark.parameterize( 'dtype2', [ np.int64, ......])

as it makes it shorter and more readable

jbrockmendel · 2018-01-24T03:16:42Z

as a style note, generally I would write this like

Noted for next time around.

jreback · 2018-01-24T11:19:08Z

pandas/tests/series/test_operators.py

-                result = op(series, other)
-                expected = alt(series, other)
+            if op == 'div':
+                alt = operator.truediv


i'd like this changed

jreback · 2018-01-24T11:19:25Z

pandas/tests/series/test_operators.py

@@ -595,77 +595,85 @@ def test_divide_decimal(self):

        assert_series_equal(expected, s)

-    def test_div(self):
+    @pytest.mark.parametrize('dtype2', [np.int64, np.int32, np.int16, np.int8,


i'd like this changed

jreback · 2018-01-25T00:54:02Z

pandas/tests/series/test_operators.py

-            result = p['first'] / p['second']
-            expected = Series(p['first'].values / p['second'].values)
-            assert_series_equal(result, expected)
+    def test_ser_div_ser_name_propagation(self):


this is a dupe of the test right above

jreback · 2018-01-25T00:54:12Z

pandas/tests/series/test_operators.py

+                                        np.float64, np.float32, np.float16,
+                                        np.uint64, np.uint32,
+                                        np.uint16, np.uint8])
+    @pytest.mark.parametrize('dtype1', [np.int64, np.float64, np.uint64])


pls clean the parameterize up here as I suggested

jreback · 2018-01-25T00:54:49Z

pandas/tests/series/test_operators.py

-                    result = rop(series, other)
-                    expected = alt(other, series)
-                    assert_almost_equal(result, expected)
+    @pytest.mark.parametrize('ts', [lambda x: (x, x * 2, False),


much nicer. if you'd format as I suggest this become very readable.

…lit_operators

jbrockmendel · 2018-01-29T16:32:17Z

I think this should be good to go. Will make the test-duplication discussed elsewhere easier.

jbrockmendel · 2018-01-31T16:22:18Z

This is now a blocker for the ongoing centralizing-of-arithmetic tests.

jreback

small change, other lgtm. ping on green.

jreback · 2018-02-01T11:37:05Z

pandas/tests/series/test_operators.py

+    @pytest.mark.parametrize(
+        'ts',
+        [
+            lambda x: (x, x * 2, False),


don't write it like this rather split out parameters

@pytest.mark.parametrize( 'ts' [ (lamda x: x, lamda x: x[::2], False), .... ])

its slightly longer but much more obvious

Sometimes I look at the results of these format changes and say "yah, he's right, that is more readable." This isn't one of those times. Doing it anyway.

…lit_operators

jbrockmendel · 2018-02-02T01:32:09Z

ping

Related: #19336 Author: Brock Mendel <[email protected]> Closes #19347 from jbrockmendel/div_zero2 and squashes the following commits: be1e2e1 [Brock Mendel] move fixture to conftest 64b0c08 [Brock Mendel] Merge branch 'master' of https://github.com/pandas-dev/pandas into div_zero2 aa969f8 [Brock Mendel] Merge branch 'master' of https://github.com/pandas-dev/pandas into div_zero2 000aefd [Brock Mendel] fix long again 9de356a [Brock Mendel] revert fixture to fix test_range failures b8cf21d [Brock Mendel] flake8 remove unused import afedba9 [Brock Mendel] whatsnew clarification b51c2e1 [Brock Mendel] fixturize 37efd51 [Brock Mendel] make zero a fixture 965f721 [Brock Mendel] Merge branch 'master' of https://github.com/pandas-dev/pandas into div_zero2 d648ef6 [Brock Mendel] requested edits 1ef3a6c [Brock Mendel] Merge branch 'master' of https://github.com/pandas-dev/pandas into div_zero2 78de1a4 [Brock Mendel] Merge branch 'master' of https://github.com/pandas-dev/pandas into div_zero2 0277d9f [Brock Mendel] add ipython output to whatsnew 5d7e3ea [Brock Mendel] Merge branch 'master' of https://github.com/pandas-dev/pandas into div_zero2 ea75c3c [Brock Mendel] ipython block 6fc61bd [Brock Mendel] elaborate docstring ca3bf42 [Brock Mendel] Whatsnew section cd54349 [Brock Mendel] move dispatch_missing to core.missing 06df02a [Brock Mendel] py3 fix 84c74c5 [Brock Mendel] remove operator.div for py3 6acc2f7 [Brock Mendel] fix missing import e0e89b9 [Brock Mendel] fix and and tests for divmod 969f342 [Brock Mendel] fix and test index division by zero

jbrockmendel · 2018-02-08T06:57:56Z

This is now a blocker for fixing series division by zero.

jreback · 2018-02-08T11:28:57Z

thanks

Related: pandas-dev#19336 Author: Brock Mendel <[email protected]> Closes pandas-dev#19347 from jbrockmendel/div_zero2 and squashes the following commits: be1e2e1 [Brock Mendel] move fixture to conftest 64b0c08 [Brock Mendel] Merge branch 'master' of https://github.com/pandas-dev/pandas into div_zero2 aa969f8 [Brock Mendel] Merge branch 'master' of https://github.com/pandas-dev/pandas into div_zero2 000aefd [Brock Mendel] fix long again 9de356a [Brock Mendel] revert fixture to fix test_range failures b8cf21d [Brock Mendel] flake8 remove unused import afedba9 [Brock Mendel] whatsnew clarification b51c2e1 [Brock Mendel] fixturize 37efd51 [Brock Mendel] make zero a fixture 965f721 [Brock Mendel] Merge branch 'master' of https://github.com/pandas-dev/pandas into div_zero2 d648ef6 [Brock Mendel] requested edits 1ef3a6c [Brock Mendel] Merge branch 'master' of https://github.com/pandas-dev/pandas into div_zero2 78de1a4 [Brock Mendel] Merge branch 'master' of https://github.com/pandas-dev/pandas into div_zero2 0277d9f [Brock Mendel] add ipython output to whatsnew 5d7e3ea [Brock Mendel] Merge branch 'master' of https://github.com/pandas-dev/pandas into div_zero2 ea75c3c [Brock Mendel] ipython block 6fc61bd [Brock Mendel] elaborate docstring ca3bf42 [Brock Mendel] Whatsnew section cd54349 [Brock Mendel] move dispatch_missing to core.missing 06df02a [Brock Mendel] py3 fix 84c74c5 [Brock Mendel] remove operator.div for py3 6acc2f7 [Brock Mendel] fix missing import e0e89b9 [Brock Mendel] fix and and tests for divmod 969f342 [Brock Mendel] fix and test index division by zero

…#19336)

separate numeric tests so we can isolate division by zero

ba9240c

jreback added Testing pandas testing functions or related to the test suite Numeric Operations Arithmetic, Comparison, and Logical operations labels Jan 22, 2018

jreback requested changes Jan 22, 2018

View reviewed changes

jbrockmendel mentioned this pull request Jan 22, 2018

fix and test index division by zero #19347

Closed

4 tasks

jbrockmendel added 3 commits January 22, 2018 09:55

fix flipped condition

c19dff1

comment

4182089

parametrize and dedup tests

6f734bc

jreback requested changes Jan 23, 2018

View reviewed changes

jreback requested changes Jan 24, 2018

View reviewed changes

parametrize with lambdas

393a6c8

jreback requested changes Jan 25, 2018

View reviewed changes

jbrockmendel added 2 commits January 24, 2018 18:22

requested formatting edits

5e1d39f

Merge branch 'master' of https://github.com/pandas-dev/pandas into sp…

9fb00cc

…lit_operators

jbrockmendel mentioned this pull request Jan 30, 2018

implement bits of numpy_helper in cython where possible #19450

Merged

jreback requested changes Feb 1, 2018

View reviewed changes

jbrockmendel added 2 commits February 1, 2018 08:27

Merge branch 'master' of https://github.com/pandas-dev/pandas into sp…

8fd7e31

…lit_operators

suggested reformat

4b78542

jreback added this to the 0.23.0 milestone Feb 8, 2018

jreback approved these changes Feb 8, 2018

View reviewed changes

jreback merged commit 34b86fd into pandas-dev:master Feb 8, 2018

jbrockmendel deleted the split_operators branch February 8, 2018 14:14

harisbal pushed a commit to harisbal/pandas that referenced this pull request Feb 28, 2018

separate numeric tests so we can isolate division by zero (pandas-dev…

e0c6c25

…#19336)

		@@ -1574,28 +1600,28 @@ def test_dt64_series_add_intlike(self, tz):


		class TestSeriesOperators(TestData):
		def test_op_method(self):

separate numeric tests so we can isolate division by zero #19336

separate numeric tests so we can isolate division by zero #19336

Conversation

jbrockmendel commented Jan 22, 2018

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

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

Choose a reason for hiding this comment

jbrockmendel commented Jan 22, 2018

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

codecov bot commented Jan 23, 2018 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbrockmendel commented Jan 24, 2018

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 Jan 29, 2018

jbrockmendel commented Jan 31, 2018

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

jbrockmendel commented Feb 2, 2018

jbrockmendel commented Feb 8, 2018

jreback commented Feb 8, 2018

codecov bot commented Jan 23, 2018 •

edited

Loading