Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixturize tests/frame/test_arithmetic #22736

Merged
merged 8 commits into from
Sep 26, 2018
Merged

Fixturize tests/frame/test_arithmetic #22736

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
9ce1860
Fixturize tests/frame/test_arithmetic
h-vetinari Sep 17, 2018
0d8aa09
PEP8 fix
h-vetinari Sep 18, 2018
1f8a5d6
Review (jreback)
h-vetinari Sep 18, 2018
1acc6b1
PEP8
h-vetinari Sep 18, 2018
054f4e9
Merge remote-tracking branch 'upstream/master' into fixturize_frame_a…
h-vetinari Sep 19, 2018
3ee3b70
Split up test_arith_flex_frame (review jreback)
h-vetinari Sep 20, 2018
b6e14b0
Review (jreback)
h-vetinari Sep 23, 2018
b0b82a7
Review (jbrockmendel)
h-vetinari Sep 26, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 10 additions & 8 deletions pandas/tests/frame/conftest.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,9 +70,10 @@ def mixed_float_frame():
Columns are ['A', 'B', 'C', 'D'].
"""
df = DataFrame(tm.getSeriesData())
df.A = df.A.astype('float16')
df.A = df.A.astype('float32')
df.B = df.B.astype('float32')
df.C = df.C.astype('float64')
df.C = df.C.astype('float16')
df.D = df.D.astype('float64')
return df


Expand All @@ -84,9 +85,10 @@ def mixed_float_frame2():
Columns are ['A', 'B', 'C', 'D'].
"""
df = DataFrame(tm.getSeriesData())
df.D = df.D.astype('float16')
df.D = df.D.astype('float32')
df.C = df.C.astype('float32')
df.B = df.B.astype('float64')
df.B = df.B.astype('float16')
df.D = df.D.astype('float64')
return df


Expand All @@ -99,10 +101,10 @@ def mixed_int_frame():
"""
df = DataFrame({k: v.astype(int)
for k, v in compat.iteritems(tm.getSeriesData())})
df.A = df.A.astype('uint8')
df.B = df.B.astype('int32')
df.C = df.C.astype('int64')
df.D = np.ones(len(df.D), dtype='uint64')
df.A = df.A.astype('int32')
df.B = np.ones(len(df.B), dtype='uint64')
df.C = df.C.astype('uint8')
df.D = df.C.astype('int64')
return df


Expand Down
197 changes: 78 additions & 119 deletions pandas/tests/frame/test_arithmetic.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,7 @@
import pytest
import numpy as np

from pandas.compat import range, PY3
import pandas.io.formats.printing as printing
from pandas.compat import range

import pandas as pd
import pandas.util.testing as tm
Expand Down Expand Up @@ -127,132 +126,92 @@ def test_df_add_flex_filled_mixed_dtypes(self):
'B': ser * 2})
tm.assert_frame_equal(result, expected)

def test_arith_flex_frame(self):
seriesd = tm.getSeriesData()
frame = pd.DataFrame(seriesd).copy()

mixed_float = pd.DataFrame({'A': frame['A'].copy().astype('float32'),
'B': frame['B'].copy().astype('float32'),
'C': frame['C'].copy().astype('float16'),
'D': frame['D'].copy().astype('float64')})

intframe = pd.DataFrame({k: v.astype(int)
for k, v in seriesd.items()})
mixed_int = pd.DataFrame({'A': intframe['A'].copy().astype('int32'),
'B': np.ones(len(intframe), dtype='uint64'),
'C': intframe['C'].copy().astype('uint8'),
'D': intframe['D'].copy().astype('int64')})

# force these all to int64 to avoid platform testing issues
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:
aliases = {}
else:
aliases = {'div': 'truediv'}
def test_arith_flex_frame(self, all_arithmetic_operators, float_frame,
mixed_float_frame):

for op in ops:
try:
alias = aliases.get(op, op)
f = getattr(operator, alias)
result = getattr(frame, op)(2 * frame)
exp = f(frame, 2 * frame)
tm.assert_frame_equal(result, exp)

# vs mix float
result = getattr(mixed_float, op)(2 * mixed_float)
exp = f(mixed_float, 2 * mixed_float)
tm.assert_frame_equal(result, exp)
_check_mixed_float(result, dtype=dict(C=None))

# vs mix int
if op in ['add', 'sub', 'mul']:
result = getattr(mixed_int, op)(2 + mixed_int)
exp = f(mixed_int, 2 + mixed_int)

# no overflow in the uint
dtype = None
if op in ['sub']:
dtype = dict(B='uint64', C=None)
elif op in ['add', 'mul']:
dtype = dict(C=None)
tm.assert_frame_equal(result, exp)
_check_mixed_int(result, dtype=dtype)

# rops
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

r_f = lambda x, y: f(y, x)
result = getattr(frame, 'r' + op)(2 * frame)
exp = r_f(frame, 2 * frame)
tm.assert_frame_equal(result, exp)

# vs mix float
result = getattr(mixed_float, op)(2 * mixed_float)
exp = f(mixed_float, 2 * mixed_float)
tm.assert_frame_equal(result, exp)
_check_mixed_float(result, dtype=dict(C=None))

result = getattr(intframe, op)(2 * intframe)
exp = f(intframe, 2 * intframe)
tm.assert_frame_equal(result, exp)

# vs mix int
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

if op in ['add', 'sub', 'mul']:
result = getattr(mixed_int, op)(2 + mixed_int)
exp = f(mixed_int, 2 + mixed_int)

# no overflow in the uint
dtype = None
if op in ['sub']:
dtype = dict(B='uint64', C=None)
elif op in ['add', 'mul']:
dtype = dict(C=None)
tm.assert_frame_equal(result, exp)
_check_mixed_int(result, dtype=dtype)
except:
printing.pprint_thing("Failing operation %r" % op)
raise

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

put comments on the prior line

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

result = getattr(float_frame, op)(2 * float_frame)
exp = f(float_frame, 2 * float_frame)
tm.assert_frame_equal(result, exp)

# vs mix float
result = getattr(mixed_float_frame, op)(2 * mixed_float_frame)
exp = f(mixed_float_frame, 2 * mixed_float_frame)
tm.assert_frame_equal(result, exp)
_check_mixed_float(result, dtype=dict(C=None))

@pytest.mark.parametrize('op', ['__add__', '__sub__', '__mul__'])
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Broke up the test like you wanted

def test_arith_flex_frame_mixed(self, op, int_frame, mixed_int_frame,
float_frame, mixed_float_frame):

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

make this a named function to avoid the tmp something like

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)
[...]

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

f = lambda x, y: tmp(y, x)
else:
f = getattr(operator, op)

# vs mix int
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

result = getattr(mixed_int_frame, op)(2 + mixed_int_frame)
exp = f(mixed_int_frame, 2 + mixed_int_frame)

# no overflow in the uint
dtype = None
if op in ['__sub__']:
dtype = dict(B='uint64', C=None)
elif op in ['__add__', '__mul__']:
dtype = dict(C=None)
tm.assert_frame_equal(result, exp)
_check_mixed_int(result, dtype=dtype)

# vs mix float
result = getattr(mixed_float_frame, op)(2 * mixed_float_frame)
exp = f(mixed_float_frame, 2 * mixed_float_frame)
tm.assert_frame_equal(result, exp)
_check_mixed_float(result, dtype=dict(C=None))

# vs plain int
result = getattr(int_frame, op)(2 * int_frame)
exp = f(int_frame, 2 * int_frame)
tm.assert_frame_equal(result, exp)

def test_arith_flex_frame_corner(self, all_arithmetic_operators,
float_frame):

op = all_arithmetic_operators

# Check that arrays with dim >= 3 raise
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you add a comment here on what this is testing

Added (and expanded)

for dim in range(3, 6):
arr = np.ones((1,) * dim)
msg = "Unable to coerce to Series/DataFrame"
with tm.assert_raises_regex(ValueError, msg):
f(frame, ndim_5)
Copy link
Contributor Author

@h-vetinari h-vetinari Sep 20, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you separate out these dimension-specific tests?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Split them out in a separate test


with tm.assert_raises_regex(ValueError, msg):
getattr(frame, op)(ndim_5)
getattr(float_frame, op)(arr)

# res_add = frame.add(frame)
# res_sub = frame.sub(frame)
# res_mul = frame.mul(frame)
# res_div = frame.div(2 * frame)

# 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))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


const_add = frame.add(1)
tm.assert_frame_equal(const_add, frame + 1)
const_add = float_frame.add(1)
tm.assert_frame_equal(const_add, float_frame + 1)

# corner cases
result = frame.add(frame[:0])
tm.assert_frame_equal(result, frame * np.nan)
result = float_frame.add(float_frame[:0])
tm.assert_frame_equal(result, float_frame * np.nan)

result = float_frame[:0].add(float_frame)
tm.assert_frame_equal(result, float_frame * np.nan)

result = frame[:0].add(frame)
tm.assert_frame_equal(result, frame * np.nan)
with tm.assert_raises_regex(NotImplementedError, 'fill_value'):
frame.add(frame.iloc[0], fill_value=3)
float_frame.add(float_frame.iloc[0], fill_value=3)

with tm.assert_raises_regex(NotImplementedError, 'fill_value'):
frame.add(frame.iloc[0], axis='index', fill_value=3)

def test_arith_flex_series(self):
arr = np.array([[1., 2., 3.],
[4., 5., 6.],
[7., 8., 9.]])
df = pd.DataFrame(arr, columns=['one', 'two', 'three'],
index=['a', 'b', 'c'])
float_frame.add(float_frame.iloc[0], axis='index', fill_value=3)

def test_arith_flex_series(self, simple_frame):
df = simple_frame

row = df.xs('a')
col = df['two']
Expand Down