Skip to content

BUG: Fix _binop for operators for serials which has more than one returns (divmod/rdivmod). #25588

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.

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

Merged
merged 7 commits into from
Mar 20, 2019
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
1 change: 1 addition & 0 deletions doc/source/whatsnew/v0.24.2.rst
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ Fixed Regressions
- Fixed regression in ``IntervalDtype`` construction where passing an incorrect string with 'Interval' as a prefix could result in a ``RecursionError``. (:issue:`25338`)
- Fixed regression in :class:`Categorical`, where constructing it from a categorical ``Series`` and an explicit ``categories=`` that differed from that in the ``Series`` created an invalid object which could trigger segfaults. (:issue:`25318`)
- Fixed pip installing from source into an environment without NumPy (:issue:`25193`)
- Fixed bug in :meth:`Series._binop` to handle binary operators that returns more than one :class:`Series` correctly (:issue:`25557`)
Copy link
Contributor

Choose a reason for hiding this comment

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

Reference :class:=Series.divmod=. These note are for users, and _binop isn't public.

You can remove the "that returns more than one Series correctly" part.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will refactor this part, thanks!


.. _whatsnew_0242.enhancements:

Expand Down
22 changes: 16 additions & 6 deletions pandas/core/series.py
Original file line number Diff line number Diff line change
Expand Up @@ -2502,6 +2502,15 @@ def _binop(self, other, func, level=None, fill_value=None):
-------
Series
"""

def mk_ret(result, new_index, name):
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 reuse anything from core/ops.py here? In particular, _construct_divmod_result.

ret = self._constructor(result, index=new_index, name=name)
ret.__finalize__(self)
if name is None:
# When name is None, __finalize__ overwrites current name
ret.name = None
return ret

if not isinstance(other, Series):
raise AssertionError('Other operand must be Series')

Expand All @@ -2519,12 +2528,13 @@ def _binop(self, other, func, level=None, fill_value=None):
with np.errstate(all='ignore'):
result = func(this_vals, other_vals)
name = ops.get_op_result_name(self, other)
result = self._constructor(result, index=new_index, name=name)
result = result.__finalize__(self)
if name is None:
# When name is None, __finalize__ overwrites current name
result.name = None
return result
if isinstance(result, tuple):
Copy link
Contributor

Choose a reason for hiding this comment

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

Hopefully replace with _construct_result and _construct_divmod_result.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for reminding me that two utilities. After _construct_result or _construct_divmod_result we still need to invoke __finalize__ to propagate metadata and overwrite name again. Any suggestion on implementing such construction in a more concise way ?

Copy link
Contributor

Choose a reason for hiding this comment

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

in core/ops.py, we call res_name = get_op_result_name(left, right). Is that an option here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

However after _construct_(divmod_)result, we still need __finalize__, which may overwrite name. Thus after __finalize__ we need assign name again.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Or should we do __finalize__ in _construct_result in core/ops.py ?

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 think it's ok to do __finalize__ in _construct_result since _construct_result is only used in core/ops.py at where the __finalize__ should be called (but not now).

Will do that.

final_result = ()
for r in result:
final_result = (*final_result, mk_ret(r, new_index, name))
else:
final_result = mk_ret(result, new_index, name)
return final_result

def combine(self, other, func, fill_value=None):
"""
Expand Down
14 changes: 14 additions & 0 deletions pandas/tests/series/test_operators.py
Original file line number Diff line number Diff line change
Expand Up @@ -741,6 +741,20 @@ def test_op_duplicate_index(self):
expected = pd.Series([11, 12, np.nan], index=[1, 1, 2])
assert_series_equal(result, expected)

def test_divmod(self):
a = Series([1, 1, 1, np.nan], index=['a', 'b', 'c', 'd'])
Copy link
Contributor

Choose a reason for hiding this comment

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

Add the GitHub issue number as a comment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will do.

b = Series([2, np.nan, 1, np.nan], index=['a', 'b', 'd', 'e'])

r1 = divmod(a, b)
Copy link
Contributor

Choose a reason for hiding this comment

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

use the format
result=
expected=
assert_series_equal

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

r2 = a.divmod(b)
assert_series_equal(r1[0], r2[0])
assert_series_equal(r1[1], r2[1])

r3 = divmod(b, a)
r4 = a.rdivmod(b)
assert_series_equal(r3[0], r4[0])
assert_series_equal(r3[1], r4[1])


class TestSeriesUnaryOps(object):
# __neg__, __pos__, __inv__
Expand Down