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

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

Merged
merged 9 commits into from
Feb 8, 2018
Merged

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

Changes from 4 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
ba9240c
separate numeric tests so we can isolate division by zero
jbrockmendel Jan 22, 2018
c19dff1
fix flipped condition
jbrockmendel Jan 22, 2018
4182089
comment
jbrockmendel Jan 22, 2018
6f734bc
parametrize and dedup tests
jbrockmendel Jan 22, 2018
393a6c8
parametrize with lambdas
jbrockmendel Jan 24, 2018
5e1d39f
requested formatting edits
jbrockmendel Jan 25, 2018
9fb00cc
Merge branch 'master' of https://github.com/pandas-dev/pandas into sp…
jbrockmendel Jan 25, 2018
8fd7e31
Merge branch 'master' of https://github.com/pandas-dev/pandas into sp…
jbrockmendel Feb 1, 2018
4b78542
suggested reformat
jbrockmendel Feb 1, 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
178 changes: 91 additions & 87 deletions pandas/tests/series/test_operators.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Copy link
Contributor

Choose a reason for hiding this comment

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

wouldn't this be better as a fixture

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

as a style note, generally I would write this like

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

as it makes it shorter and more readable

Copy link
Contributor

Choose a reason for hiding this comment

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

i'd like this changed

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

Choose a reason for hiding this comment

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

pls clean the parameterize up here as I suggested

def test_ser_div_ser(self, dtype1, dtype2):
# no longer do integer div for any ops, but deal with the 0's
first = Series([3, 4, 5, 8], name='first').astype(dtype1)
second = Series([0, 0, 0, 3], name='second').astype(dtype2)

with np.errstate(all='ignore'):
# no longer do integer div for any ops, but deal with the 0's
p = DataFrame({'first': [3, 4, 5, 8], 'second': [0, 0, 0, 3]})
result = p['first'] / p['second']
expected = Series(
p['first'].values.astype(float) / p['second'].values,
dtype='float64')
expected.iloc[0:3] = np.inf
assert_series_equal(result, expected)
expected = Series(first.values.astype(np.float64) / second.values,
dtype='float64')
expected.iloc[0:3] = np.inf

result = p['first'] / 0
expected = Series(np.inf, index=p.index, name='first')
assert_series_equal(result, expected)
result = first / second
assert_series_equal(result, expected)

p = p.astype('float64')
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):
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

this is a dupe of the test right above

first = Series([3, 4, 5, 8], name='first')
second = Series([1, 1, 1, 1], name='second')

p = DataFrame({'first': [3, 4, 5, 8], 'second': [1, 1, 1, 1]})
result = p['first'] / p['second']
assert_series_equal(result, p['first'].astype('float64'),
check_names=False)
assert result.name is None
assert not result.equals(p['second'] / p['first'])

# inf signing
s = Series([np.nan, 1., -1.])
result = s / 0
expected = Series([np.nan, np.inf, -np.inf])
assert_series_equal(result, expected)
expected = Series([3, 4, 5, 8], dtype='float64', name=None)
result = first / second
assert_series_equal(result, expected)
assert not result.equals(second / first)

# float/integer issue
# GH 7785
p = DataFrame({'first': (1, 0), 'second': (-0.01, -0.02)})
expected = Series([-0.01, -np.inf])
def test_div_equiv_binop(self):
# Test Series.div as well as Series.__div__
# float/integer issue
# GH#7785
first = pd.Series([1, 0], name='first')
second = pd.Series([-0.01, -0.02], name='second')
expected = Series([-0.01, -np.inf])

result = p['second'].div(p['first'])
assert_series_equal(result, expected, check_names=False)
result = second.div(first)
assert_series_equal(result, expected, check_names=False)

result = p['second'] / p['first']
assert_series_equal(result, expected)
result = second / first
assert_series_equal(result, expected)

# GH 9144
s = Series([-1, 0, 1])
def test_rdiv_zero_compat(self):
Copy link
Contributor

Choose a reason for hiding this comment

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

parametrie over 0 with various int/uint dtypes

# GH#8674
zero_array = np.array([0] * 5)
data = np.random.randn(5)
expected = pd.Series([0.] * 5)

result = 0 / s
expected = Series([0.0, nan, 0.0])
assert_series_equal(result, expected)
result = zero_array / pd.Series(data)
assert_series_equal(result, expected)

result = s / 0
expected = Series([-inf, nan, inf])
assert_series_equal(result, expected)
result = pd.Series(zero_array) / data
assert_series_equal(result, expected)

result = s // 0
expected = Series([-inf, nan, inf])
assert_series_equal(result, expected)
result = pd.Series(zero_array) / pd.Series(data)
assert_series_equal(result, expected)

# GH 8674
zero_array = np.array([0] * 5)
data = np.random.randn(5)
expected = pd.Series([0.] * 5)
result = zero_array / pd.Series(data)
assert_series_equal(result, expected)
def test_div_zero_inf_signs(self):
# GH#9144, inf signing
ser = Series([-1, 0, 1], name='first')
expected = Series([-np.inf, np.nan, np.inf], name='first')

result = pd.Series(zero_array) / data
assert_series_equal(result, expected)
result = ser / 0
assert_series_equal(result, expected)

result = pd.Series(zero_array) / pd.Series(data)
assert_series_equal(result, expected)
def test_rdiv_zero(self):
Copy link
Contributor

Choose a reason for hiding this comment

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

parametrize

# GH#9144
ser = Series([-1, 0, 1], name='first')
expected = Series([0.0, np.nan, 0.0], name='first')

result = 0 / ser
assert_series_equal(result, expected)

def test_floordiv_div(self):
Copy link
Contributor

Choose a reason for hiding this comment

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

same. see the pattern :>

need to create a zero fixture

# GH#9144
ser = Series([-1, 0, 1], name='first')

result = ser // 0
expected = Series([-inf, nan, inf], name='first')
assert_series_equal(result, expected)


class TestTimedeltaSeriesArithmeticWithIntegers(object):
Expand Down Expand Up @@ -1574,28 +1582,29 @@ def test_dt64_series_add_intlike(self, tz):


class TestSeriesOperators(TestData):
def test_op_method(self):
Copy link
Contributor

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 to describe what tests this class does

def check(series, other, check_reverse=False):
simple_ops = ['add', 'sub', 'mul', 'floordiv', 'truediv', 'pow']
if not compat.PY3:
simple_ops.append('div')

for opname in simple_ops:
op = getattr(Series, opname)
@pytest.mark.parametrize('opname', ['add', 'sub', 'mul', 'floordiv',
'truediv', 'div', 'pow'])
def test_op_method(self, opname):
# check that Series.{opname} behaves like Series.__{opname}__,
if opname == 'div' and compat.PY3:
pytest.skip('div test only for Py3')

if op == 'div':
alt = operator.truediv
else:
alt = getattr(operator, opname)
def check(series, other, check_reverse=False):
op = getattr(Series, opname)

result = op(series, other)
expected = alt(series, other)
if op == 'div':
alt = operator.truediv
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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...

Copy link
Contributor

Choose a reason for hiding this comment

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

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)

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

i'd like this changed

else:
alt = getattr(operator, opname)

result = op(series, other)
expected = alt(series, other)
assert_almost_equal(result, expected)
if check_reverse:
rop = getattr(Series, "r" + opname)
result = rop(series, other)
expected = alt(other, series)
assert_almost_equal(result, expected)
if check_reverse:
rop = getattr(Series, "r" + opname)
result = rop(series, other)
expected = alt(other, series)
assert_almost_equal(result, expected)

check(self.ts, self.ts * 2)
check(self.ts, self.ts[::2])
Expand Down Expand Up @@ -1992,20 +2001,15 @@ def test_operators_corner(self):
index=self.ts.index[:-5], name='ts')
tm.assert_series_equal(added[:-5], expected)

def test_operators_reverse_object(self):
@pytest.mark.parametrize('op', [operator.add, operator.sub, operator.mul,
operator.truediv, operator.floordiv])
def test_operators_reverse_object(self, op):
# GH 56
arr = Series(np.random.randn(10), index=np.arange(10), dtype=object)

def _check_op(arr, op):
result = op(1., arr)
expected = op(1., arr.astype(float))
assert_series_equal(result.astype(float), expected)

_check_op(arr, operator.add)
_check_op(arr, operator.sub)
_check_op(arr, operator.mul)
_check_op(arr, operator.truediv)
_check_op(arr, operator.floordiv)
result = op(1., arr)
expected = op(1., arr.astype(float))
assert_series_equal(result.astype(float), expected)

def test_arith_ops_df_compat(self):
# GH 1134
Expand Down