Fixturize tests/frame/test_arithmetic #22736

h-vetinari · 2018-09-17T20:41:00Z

Split off from #22730 as per review from @WillAyd

The changes in conftest.py are due to the following:

In translating the quasi-fixtures from TestData to conftest in #22236, I sorted the dtypes for the columns of mixed_float_frame and mixed_int_frame, which turns out to have been a mistake. This is reverted here to be a true translation of the attribute of TestData. Otherwise, tests in the newly fixturized test_arithmetic.py would fail.

pep8speaks · 2018-09-17T20:41:03Z

Hello @h-vetinari! Thanks for updating the PR.

There are no PEP8 issues in the file pandas/tests/frame/conftest.py !
There are no PEP8 issues in the file pandas/tests/frame/test_arithmetic.py !

Comment last updated on September 20, 2018 at 10:06 Hours UTC

jreback · 2018-09-18T11:52:04Z

pandas/tests/frame/test_arithmetic.py

-    def test_arith_flex_frame(self):
-        seriesd = tm.getSeriesData()
-        frame = pd.DataFrame(seriesd).copy()
+    @pytest.mark.parametrize('op', ['add', 'sub', 'mul', 'div', 'truediv',


can you change these to use the fixture: all_arithmetic_operators

jreback · 2018-09-18T11:52:30Z

pandas/tests/frame/test_arithmetic.py

-        intframe = pd.DataFrame({c: s for c, s in intframe.items()},
-                                dtype=np.int64)
-
-        ops = ['add', 'sub', 'mul', 'div', 'truediv', 'pow', 'floordiv', 'mod']
        if not PY3:


you prob don't need this (when using the fixture)

jreback · 2018-09-18T11:52:55Z

pandas/tests/frame/test_arithmetic.py

+        tm.assert_frame_equal(result, exp)
+        _check_mixed_float(result, dtype=dict(C=None))
+
+        # vs mix int


might be more clear to move these to a separate test (from here down)

I don't want to get into orthogonal changes here. I haven't changed anything about this except removing the indentation. This goes for the test split requested below as well.

jreback · 2018-09-18T11:53:52Z

pandas/tests/frame/test_arithmetic.py

-
-        const_add = frame.add(1)
-        tm.assert_frame_equal(const_add, frame + 1)
+        # ndim >= 3


here and down could be in the top-section, e.g. the first test as they test all ops (or another tests is even better, with the fixture this becomes easy)

jreback · 2018-09-18T11:54:06Z

pandas/tests/frame/test_arithmetic.py

-        const_add = frame.add(1)
-        tm.assert_frame_equal(const_add, frame + 1)
+        # ndim >= 3
+        ndim_5 = np.ones(float_frame.shape + (3, 4, 5))


can you add a comment here on what this is testing

See above - I did not write the test; just removed indentation

jreback · 2018-09-18T14:07:03Z

@h-vetinari ping when updated

h-vetinari · 2018-09-18T16:52:21Z

@jreback

I though your request for all_arithmetic_operators would not change anything, but it includes more operators than previously tested - I get errors for all the reverse operators:

AttributeError: module 'operator' has no attribute '__rmod__'  [etc.]

I've skipped those tests now.

codecov · 2018-09-18T20:16:47Z

Codecov Report

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

@@            Coverage Diff             @@
##           master   #22736      +/-   ##
==========================================
+ Coverage   92.17%   92.18%   +0.01%     
==========================================
  Files         169      169              
  Lines       50778    50819      +41     
==========================================
+ Hits        46807    46850      +43     
+ Misses       3971     3969       -2

Flag	Coverage Δ
#multiple	`90.6% <ø> (+0.01%)`	⬆️
#single	`42.37% <ø> (+0.04%)`	⬆️

Impacted Files	Coverage Δ
pandas/util/testing.py	`85.82% <0%> (-0.21%)`	⬇️
pandas/io/formats/format.py	`98.35% <0%> (-0.01%)`	⬇️
pandas/core/strings.py	`98.63% <0%> (ø)`	⬆️
pandas/core/dtypes/base.py	`100% <0%> (ø)`	⬆️
pandas/core/generic.py	`96.67% <0%> (ø)`	⬆️
pandas/core/reshape/melt.py	`97.34% <0%> (ø)`	⬆️
pandas/core/frame.py	`97.2% <0%> (ø)`	⬆️
pandas/core/indexes/multi.py	`95.45% <0%> (ø)`	⬆️
pandas/io/formats/excel.py	`97.4% <0%> (ø)`	⬆️
pandas/core/arrays/categorical.py	`95.75% <0%> (+0.01%)`	⬆️
... and 11 more

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 1c113db...b0b82a7. Read the comment docs.

…rithmetic

h-vetinari

@ jreback

might be more clear to move these to a separate test (from here down)

can you add a comment here on what this is testing

OK, I relented, and split up and cleaned up that huge test. Also the skips for the r-versions of the ops aren't necessary anymore. They were tested for add/sub/mult before, and I extended that to all ops now.

h-vetinari · 2018-09-20T08:07:23Z

pandas/tests/frame/test_arithmetic.py

-                    tm.assert_frame_equal(result, exp)
-                    _check_mixed_int(result, dtype=dtype)
-
-                    # rops


rops were only tested within that if-block. Now they're all tested

h-vetinari · 2018-09-20T08:07:28Z

pandas/tests/frame/test_arithmetic.py

-                    exp = f(intframe, 2 * intframe)
-                    tm.assert_frame_equal(result, exp)
-
-                    # vs mix int


For some reason, this part was tested twice. only once in cleaned-up version

h-vetinari · 2018-09-20T08:07:51Z

pandas/tests/frame/test_arithmetic.py

+        tm.assert_frame_equal(result, exp)
+        _check_mixed_float(result, dtype=dict(C=None))
+
+    @pytest.mark.parametrize('op', ['__add__', '__sub__', '__mul__'])


Broke up the test like you wanted

h-vetinari · 2018-09-20T08:08:48Z

pandas/tests/frame/test_arithmetic.py

+
+        op = all_arithmetic_operators
+
+        # Check that arrays with dim >= 3 raise


can you add a comment here on what this is testing

Added (and expanded)

h-vetinari · 2018-09-20T08:09:24Z

pandas/tests/frame/test_arithmetic.py

            msg = "Unable to coerce to Series/DataFrame"
            with tm.assert_raises_regex(ValueError, msg):
-                f(frame, ndim_5)


This part (testing via the f of the op) is removed because it's testing the same thing as testing the op from the frame directly below

Can you separate out these dimension-specific tests?

Split them out in a separate test

h-vetinari · 2018-09-21T15:08:58Z

@jreback Green

jreback

ok test looks a lot nicer. small change, ping on green.

jreback · 2018-09-23T12:43:07Z

pandas/tests/frame/test_arithmetic.py

-
-            # ndim >= 3
-            ndim_5 = np.ones(frame.shape + (3, 4, 5))
+        op = all_arithmetic_operators  # one instance of parametrized fixture


put comments on the prior line

jreback · 2018-09-23T12:45:52Z

pandas/tests/frame/test_arithmetic.py

+
+        if op.startswith('__r'):
+            # get op without "r" and invert it
+            tmp = getattr(operator, op[:2] + op[3:])


make this a named function to avoid the tmp something like

def f(x, y): # comment op = op.replace('__r', '__') return getattr(operator, op)

this comment applies to the prior test actually. I don't think you need this here at all, as op is just add, mul, sub.

did you mean to test the reverse of these as well?

make this a named function to avoid the tmp something like [...]

Best I can come up with is the following. Is this what you meant?

[...] # one instance of parametrized fixture op = all_arithmetic_operators def f(x, y): if op.startswith('__r'): # get op without "r" and invert it return getattr(operator, op.replace('__r', '__'))(y, x) return getattr(operator, op)(x, y) [...]

this comment applies to the prior test actually. I don't think you need this here at all, as op is just add, mul, sub. did you mean to test the reverse of these as well?

That was indeed not necessary anymore. Removed

ok this is fine (your change). note that I suspect we do this in other places, maybe see if you can make a more generalized version of this that we can then include in the pandas.util.testing (but followon-PR)

jreback · 2018-09-23T12:48:24Z

pandas/tests/frame/test_arithmetic.py

+
+        if op.startswith('__r'):
+            # get op without "r" and invert it
+            tmp = getattr(operator, op[:2] + op[3:])


this comment applies to the prior test actually. I don't think you need this here at all, as op is just add, mul, sub.

did you mean to test the reverse of these as well?

jreback · 2018-09-23T13:07:54Z

pandas/tests/frame/test_arithmetic.py

+        def f(x, y):
+            if op.startswith('__r'):
+                # get op without "r" and invert it
+                return getattr(operator, op.replace('__r', '__'))(y, x)


@jbrockmendel pls have a look here

This is fine. The alternative would be to do a lookup in core.ops, but I think for a reader who is unfamiliar with core.ops this is clearer.

Modified comment slightly

jreback · 2018-09-23T13:08:20Z

ok by me. @jbrockmendel if you'd have a look and merge if ok.

jbrockmendel · 2018-09-26T00:06:22Z

pandas/tests/frame/test_arithmetic.py

-        # tm.assert_frame_equal(res_add, frame + frame)
-        # tm.assert_frame_equal(res_sub, frame - frame)
-        # tm.assert_frame_equal(res_mul, frame * frame)
-        # tm.assert_frame_equal(res_div, frame / (2 * frame))


Did this commented-out stuff go somewhere else? Or just determined to be redundant?

Determined to be redundant with test_arith_flex_frame, which checks all arith_ops, and much more thoroughly.

jreback · 2018-09-26T10:06:21Z

thanks @h-vetinari

so this is a good example of a change. you moved / parameterized things, but its a single change that is quite straightforward for a reviewer to grok / comment on w/o diving into things too much.

h-vetinari mentioned this pull request Sep 17, 2018

CLN: res/exp and GH references in frame tests #22730

Merged

3 tasks

Fixturize tests/frame/test_arithmetic

9ce1860

h-vetinari force-pushed the fixturize_frame_arithmetic branch from 8d5c307 to 9ce1860 Compare September 17, 2018 23:39

h-vetinari mentioned this pull request Sep 17, 2018

TST/CLN: remove TestData from frame-tests; replace with fixtures #22471

Closed

34 tasks

PEP8 fix

0d8aa09

jreback requested changes Sep 18, 2018

View reviewed changes

jreback added Testing pandas testing functions or related to the test suite Clean labels Sep 18, 2018

jreback added this to the 0.24.0 milestone Sep 18, 2018

Review (jreback)

1f8a5d6

PEP8

1acc6b1

Merge remote-tracking branch 'upstream/master' into fixturize_frame_a…

054f4e9

…rithmetic

h-vetinari commented Sep 20, 2018

View reviewed changes

Split up test_arith_flex_frame (review jreback)

3ee3b70

h-vetinari force-pushed the fixturize_frame_arithmetic branch from 08fae94 to 3ee3b70 Compare September 20, 2018 10:06

h-vetinari mentioned this pull request Sep 20, 2018

CI: Improve CircleCI #22763

Closed

jreback requested changes Sep 23, 2018

View reviewed changes

jreback requested a review from jbrockmendel September 23, 2018 12:48

Review (jreback)

b6e14b0

jreback reviewed Sep 23, 2018

View reviewed changes

jreback approved these changes Sep 23, 2018

View reviewed changes

jbrockmendel reviewed Sep 26, 2018

View reviewed changes

Review (jbrockmendel)

b0b82a7

jreback merged commit 4a459b8 into pandas-dev:master Sep 26, 2018

Sup3rGeo pushed a commit to Sup3rGeo/pandas that referenced this pull request Oct 1, 2018

Fixturize tests/frame/test_arithmetic (pandas-dev#22736)

22b6123

h-vetinari deleted the fixturize_frame_arithmetic branch October 5, 2018 16:22


		op = all_arithmetic_operators

		# Check that arrays with dim >= 3 raise

Fixturize tests/frame/test_arithmetic #22736

Fixturize tests/frame/test_arithmetic #22736

Conversation

h-vetinari commented Sep 17, 2018 • edited Loading

pep8speaks commented Sep 17, 2018 • edited Loading

Comment last updated on September 20, 2018 at 10:06 Hours UTC

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 Sep 18, 2018

h-vetinari commented Sep 18, 2018 • edited Loading

codecov bot commented Sep 18, 2018 • edited Loading

Codecov Report

h-vetinari 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

Choose a reason for hiding this comment

h-vetinari Sep 20, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

h-vetinari commented Sep 21, 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

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 Sep 23, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jreback commented Sep 26, 2018

h-vetinari commented Sep 17, 2018 •

edited

Loading

pep8speaks commented Sep 17, 2018 •

edited

Loading

h-vetinari commented Sep 18, 2018 •

edited

Loading

codecov bot commented Sep 18, 2018 •

edited

Loading

h-vetinari Sep 20, 2018 •

edited

Loading