Skip to content

Remove unused/unreachable code from ops #18964

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

Closed
wants to merge 14 commits into from
Closed

Remove unused/unreachable code from ops #18964

Changes from 1 commit
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
65dd8b3
remove unused code, impossible cases. add todo comments
jbrockmendel Dec 28, 2017
f65eab4
add tests for op names
jbrockmendel Dec 29, 2017
9091386
Merge branch 'master' of https://github.com/pandas-dev/pandas into op…
jbrockmendel Dec 29, 2017
5a5d34a
trim the set of tested funcnames
jbrockmendel Dec 29, 2017
38d6c08
troubleshooting, add tests that belong elsewhere
jbrockmendel Dec 29, 2017
c712bfc
fix Series op signatures
jbrockmendel Dec 29, 2017
4a2a021
cleanup commented-out bits from troubleshooting
jbrockmendel Dec 29, 2017
e53b7fa
cleanup leftover troubleshooting relics
jbrockmendel Dec 29, 2017
13ebe0a
Merge branch 'master' of https://github.com/pandas-dev/pandas into op…
jbrockmendel Dec 29, 2017
f37eb9d
Merge branch 'master' of https://github.com/pandas-dev/pandas into op…
jbrockmendel Dec 30, 2017
58f7ef7
Merge branch 'master' of https://github.com/pandas-dev/pandas into op…
jbrockmendel Dec 31, 2017
d8305c1
Merge branch 'master' of https://github.com/pandas-dev/pandas into op…
jbrockmendel Dec 31, 2017
fdf461e
requested edits
jbrockmendel Dec 31, 2017
5b704fa
Merge branch 'master' of https://github.com/pandas-dev/pandas into op…
jbrockmendel Jan 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
97 changes: 24 additions & 73 deletions pandas/core/ops.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -156,6 +156,7 @@ def add_methods(cls, new_methods, force, select, exclude):
raise TypeError("May only pass either select or exclude")

if select:
# TODO: This is not hit in coverage report. Is it still needed?
select = set(select)
methods = {}
for key, method in new_methods.items():
Expand All @@ -164,6 +165,7 @@ def add_methods(cls, new_methods, force, select, exclude):
new_methods = methods

if exclude:
# TODO: This is not hit in coverage report. Is it still needed?
for k in exclude:
new_methods.pop(k, None)

Expand Down Expand Up @@ -360,8 +362,8 @@ class _TimeOp(_Op):
def __init__(self, left, right, name, na_op):
super(_TimeOp, self).__init__(left, right, name, na_op)

lvalues = self._convert_to_array(left, name=name)
rvalues = self._convert_to_array(right, name=name, other=lvalues)
lvalues = self._convert_to_array(left)
rvalues = self._convert_to_array(right, other=lvalues)

# left
self.is_offset_lhs = is_offsetlike(left)
Expand Down Expand Up @@ -440,44 +442,11 @@ def _validate_timedelta(self, name):
'of a series/ndarray of type datetime64[ns] '
'or a timedelta')

def _validate_offset(self, name):
# assumes self.is_offset_lhs

if self.is_timedelta_rhs:
# 2 timedeltas
if name not in ('__div__', '__rdiv__', '__truediv__',
'__rtruediv__', '__add__', '__radd__', '__sub__',
'__rsub__'):
raise TypeError("can only operate on a timedeltas for addition"
", subtraction, and division, but the operator"
" [{name}] was passed".format(name=name))

elif self.is_datetime_rhs:
if name not in ('__add__', '__radd__'):
raise TypeError("can only operate on a timedelta/DateOffset "
"and a datetime for addition, but the operator"
" [{name}] was passed".format(name=name))

else:
raise TypeError('cannot operate on a series without a rhs '
'of a series/ndarray of type datetime64[ns] '
'or a timedelta')

def _validate(self, lvalues, rvalues, name):
if self.is_datetime_lhs:
return self._validate_datetime(lvalues, rvalues, name)
elif self.is_timedelta_lhs:
return self._validate_timedelta(name)
elif self.is_offset_lhs:
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 offset case not possible? (it may not be tested) but is it possible?

Copy link
Member Author

Choose a reason for hiding this comment

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

Because _TimeOp is only used if is_datetime_lhs or is_timedelta_lhs

Copy link
Contributor

Choose a reason for hiding this comment

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

ok, can you doc-string things a bit, will help future readers.

return self._validate_offset(name)

if ((self.is_integer_lhs or self.is_floating_lhs) and
self.is_timedelta_rhs):
self._check_timedelta_with_numeric(name)
else:
raise TypeError('cannot operate on a series without a rhs '
'of a series/ndarray of type datetime64[ns] '
'or a timedelta')

def _check_timedelta_with_numeric(self, name):
if name not in ('__div__', '__truediv__', '__mul__', '__rmul__'):
Expand All @@ -486,9 +455,10 @@ def _check_timedelta_with_numeric(self, name):
"multiplication, but the operator [{name}] "
"was passed".format(name=name))

def _convert_to_array(self, values, name=None, other=None):
def _convert_to_array(self, values, other=None):
"""converts values to ndarray"""
from pandas.core.tools.timedeltas import to_timedelta
name = self.name

ovalues = values
supplied_dtype = None
Expand Down Expand Up @@ -517,15 +487,16 @@ def _convert_to_array(self, values, name=None, other=None):
elif isinstance(values, pd.DatetimeIndex):
values = values.to_series()
# datetime with tz
Copy link
Contributor

Choose a reason for hiding this comment

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

fix this comment

elif (isinstance(ovalues, datetime.datetime) and
hasattr(ovalues, 'tzinfo')):
elif isinstance(ovalues, datetime.datetime):
values = pd.DatetimeIndex(values)
# datetime array with tz
elif is_datetimetz(values):
if isinstance(values, ABCSeries):
values = values._values
elif not (isinstance(values, (np.ndarray, ABCSeries)) and
is_datetime64_dtype(values)):
# TODO: This is not hit in tests. What case is it intended
Copy link
Contributor

Choose a reason for hiding this comment

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

I think can remove this case if change 492 to
elif isinstance(ovalues, (datetime.datetime, np.datetime64))

In [4]: np.datetime64('2013-01-01T00:00:00.000000000')
Out[4]: numpy.datetime64('2013-01-01T00:00:00.000000000')

In [5]: isinstance(np.datetime64('2013-01-01T00:00:00.000000000'), np.ndarray)
Out[5]: False

In [6]: isinstance(np.array(np.datetime64('2013-01-01T00:00:00.000000000')), np.ndarray)
Out[6]: True

In [7]: np.array(np.datetime64('2013-01-01T00:00:00.000000000'))
Out[7]: array(1356998400000000000, dtype='datetime64[ns]')

Copy link
Member Author

Choose a reason for hiding this comment

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

Sounds good.

# to catch?
values = libts.array_to_datetime(values)
elif inferred_type in ('timedelta', 'timedelta64'):
# have a timedelta, convert to to ns here
Expand Down Expand Up @@ -566,8 +537,6 @@ def _convert_for_datetime(self, lvalues, rvalues):
if self.is_datetime_lhs and self.is_datetime_rhs:
if self.name in ('__sub__', '__rsub__'):
self.dtype = 'timedelta64[ns]'
else:
self.dtype = 'datetime64[ns]'
elif self.is_datetime64tz_lhs:
self.dtype = lvalues.dtype
elif self.is_datetime64tz_rhs:
Expand All @@ -590,9 +559,7 @@ def _offset(lvalues, rvalues):
self.na_op = lambda x, y: getattr(x, self.name)(y)
return lvalues, rvalues

if self.is_offset_lhs:
lvalues, rvalues = _offset(lvalues, rvalues)
elif self.is_offset_rhs:
if self.is_offset_rhs:
rvalues, lvalues = _offset(rvalues, lvalues)
else:

Expand All @@ -611,8 +578,6 @@ def _offset(lvalues, rvalues):
self.dtype = 'timedelta64[ns]'

# convert Tick DateOffset to underlying delta
if self.is_offset_lhs:
lvalues = to_timedelta(lvalues, box=False)
if self.is_offset_rhs:
rvalues = to_timedelta(rvalues, box=False)

Expand Down Expand Up @@ -721,6 +686,10 @@ def safe_na_op(lvalues, rvalues):
return na_op(lvalues, rvalues)
except Exception:
if isinstance(rvalues, ABCSeries):
# TODO: This case is not hit in tests. For it to be hit would
# require `right` to be an object such that right.values
Copy link
Contributor

Choose a reason for hiding this comment

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

limit the comments to useful info, 'screwup' does not fall in this category

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 comment is more a suggestion that we get rid of this block.

Copy link
Contributor

Choose a reason for hiding this comment

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

I get that. but this is public code, you can certainly say 'this should be 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 with removing this code here

# is a Series, which is likely an indication that a screwup
# has occurred somewhere.
if is_object_dtype(rvalues):
# if dtype is object, try elementwise op
return libalgos.arrmap_object(rvalues,
Expand All @@ -731,7 +700,9 @@ def safe_na_op(lvalues, rvalues):
lambda x: op(x, rvalues))
raise

def wrapper(left, right, name=name, na_op=na_op):
def wrapper(left, right, na_op=na_op):
# TODO: `na_op` does not belong in Series.__{op}__ signature. But this
Copy link
Contributor

Choose a reason for hiding this comment

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

huh?

Copy link
Member Author

Choose a reason for hiding this comment

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

0.21.1:

>>> inspect.getargspec(pd.Series.__add__)
ArgSpec(args=['left', 'right', 'name', 'na_op'], varargs=None, keywords=None, defaults=('__add__', <function na_op at 0x10c4b7d70>))

Copy link
Contributor

Choose a reason for hiding this comment

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

and so what?

Copy link
Member Author

Choose a reason for hiding this comment

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

... and there's precedent for caring about signatures that make sense, especially in super-commonly-used user-facing methods.

Copy link
Member Author

Choose a reason for hiding this comment

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

Don't worry about it anyway, I found a way to get na_op and name out of the signature.

# is needed here for closure/namespace purposes ATM.
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 doc-strings. this is extremely useful to future readers.

Copy link
Member Author

Choose a reason for hiding this comment

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

Any docstring added here would become the docstring for pd.Series.__foo__, which I doubt is what you have in mind here.

Copy link
Contributor

Choose a reason for hiding this comment

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

right, so just a comment then which describes the input & types


if isinstance(right, ABCDataFrame):
return NotImplemented
Expand All @@ -740,32 +711,27 @@ def wrapper(left, right, name=name, na_op=na_op):

converted = _Op.get_op(left, right, name, na_op)

left, right = converted.left, converted.right
lvalues, rvalues = converted.lvalues, converted.rvalues
dtype = converted.dtype
wrap_results = converted.wrap_results
na_op = converted.na_op

if isinstance(rvalues, ABCSeries):
name = _maybe_match_name(left, rvalues)
res_name = _maybe_match_name(left, rvalues)
lvalues = getattr(lvalues, 'values', lvalues)
rvalues = getattr(rvalues, 'values', rvalues)
# _Op aligns left and right
else:
name = left.name
res_name = left.name
if (hasattr(lvalues, 'values') and
not isinstance(lvalues, pd.DatetimeIndex)):
lvalues = lvalues.values

result = wrap_results(safe_na_op(lvalues, rvalues))
return construct_result(
left,
result,
index=left.index,
name=name,
dtype=dtype,
)
return construct_result(left, result,
index=left.index, name=res_name, dtype=dtype)

wrapper.__name__ = name
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe this is hit in tests, yes? (the name of the wrapper)

Copy link
Member Author

@jbrockmendel jbrockmendel Dec 28, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

0.21.1:

>>> pd.Series.__sub__
<unbound method Series.wrapper>

Copy link
Contributor

Choose a reason for hiding this comment

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

hmm yeah that's not right. can you add tests for this? (I believe we have a suite of tests for the stat methods, so prob just need to add these on).

Copy link
Member Author

Choose a reason for hiding this comment

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

We recently made similar tests for Index comparison method names in tests.indexes.test_base, I don't see much else to that effect. I'll put something together.

Copy link
Contributor

Choose a reason for hiding this comment

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

https://github.com/pandas-dev/pandas/blob/master/pandas/tests/generic/test_generic.py#L507

return wrapper


Expand Down Expand Up @@ -849,6 +815,8 @@ def wrapper(self, other, axis=None):
# Validate the axis parameter
if axis is not None:
self._get_axis_number(axis)
Copy link
Contributor

Choose a reason for hiding this comment

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

yes should change (but agreed may not be hit in tests)

# TODO: should this be `axis = self._get_axis_number(axis)`?
# This is not hit in tests.

if isinstance(other, ABCSeries):
name = _maybe_match_name(self, other)
Expand Down Expand Up @@ -1371,23 +1339,6 @@ def f(self, other):

def _arith_method_PANEL(op, name, str_rep=None, fill_zeros=None,
default_axis=None, **eval_kwargs):
# copied from Series na_op above, but without unnecessary branch for
# non-scalar
Copy link
Contributor

Choose a reason for hiding this comment

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

ok, not super concerned about Panel things generally.

def na_op(x, y):
import pandas.core.computation.expressions as expressions

try:
result = expressions.evaluate(op, str_rep, x, y, **eval_kwargs)
except TypeError:

# TODO: might need to find_common_type here?
result = np.empty(len(x), dtype=x.dtype)
mask = notna(x)
result[mask] = op(x[mask], y)
result, changed = maybe_upcast_putmask(result, ~mask, np.nan)

result = missing.fill_zeros(result, x, y, name, fill_zeros)
return result

# work only for scalars
def f(self, other):
Expand Down