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.

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

BUG: divmod return type #22932

Merged
merged 15 commits into from
Oct 8, 2018
15 changes: 12 additions & 3 deletions doc/source/extending.rst
Original file line number Diff line number Diff line change
Expand Up @@ -160,9 +160,18 @@ your ``MyExtensionArray`` class, as follows:
MyExtensionArray._add_arithmetic_ops()
MyExtensionArray._add_comparison_ops()

Note that since ``pandas`` automatically calls the underlying operator on each
element one-by-one, this might not be as performant as implementing your own
version of the associated operators directly on the ``ExtensionArray``.

.. note::

Since ``pandas`` automatically calls the underlying operator on each
element one-by-one, this might not be as performant as implementing your own
version of the associated operators directly on the ``ExtensionArray``.

This implementation will try to reconstruct a new ``ExtensionArray`` with the
result of the element-wise operation. Whether or not that succeeds depends on
whether the operation returns a result that's valid for the ``ExtensionArray``.
If an ``ExtensionArray`` cannot be reconstructed, a list containing the scalars
returned instead.
Copy link
Member

Choose a reason for hiding this comment

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

I find it a bit strange we return a list and not an array ?
(but that's maybe off topic for this PR)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I found that strange as well. An array would be better to return.

This is new in 0.24 right? If so, I'll just make the change 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.

I have another PR touching about the same place right now, so I'm going to hold off on changing that till later.


.. _extending.extension.testing:

Expand Down
16 changes: 12 additions & 4 deletions pandas/core/arrays/base.py
Original file line number Diff line number Diff line change
Expand Up @@ -775,10 +775,18 @@ def convert_values(param):
res = [op(a, b) for (a, b) in zip(lvalues, rvalues)]

if coerce_to_dtype:
try:
res = self._from_sequence(res)
except TypeError:
pass
if op.__name__ in {'divmod', 'rdivmod'}:
try:
a, b = zip(*res)
Copy link
Member

Choose a reason for hiding this comment

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

is the zip-star necessary? If we get here shouldn't res just be a 2-tuple? so a, b = res?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

right now res is a list of tuples, so I think we have to split each of those with a zip(*)

> /Users/taugspurger/sandbox/pandas/pandas/core/arrays/base.py(781)_binop()
    779                     try:
    780                         import pdb; pdb.set_trace()
--> 781                         a, b = zip(*res)
    782                         res = (self._from_sequence(a),
    783                                self._from_sequence(b))

ipdb> res
[(Decimal('0'), Decimal('1')), (Decimal('1'), Decimal('0')), (Decimal('1'), Decimal('1')), (Decimal('2'), Decimal('0'))]
ipdb> n
> /Users/taugspurger/sandbox/pandas/pandas/core/arrays/base.py(782)_binop()
    780                         import pdb; pdb.set_trace()
    781                         a, b = zip(*res)
--> 782                         res = (self._from_sequence(a),
    783                                self._from_sequence(b))
    784                     except TypeError:

ipdb> p a, b
((Decimal('0'), Decimal('1'), Decimal('1'), Decimal('2')), (Decimal('1'), Decimal('0'), Decimal('1'), Decimal('0')))

res = (self._from_sequence(a),
self._from_sequence(b))
except TypeError:
pass
else:
try:
res = self._from_sequence(res)
except TypeError:
pass

return res

Expand Down
4 changes: 4 additions & 0 deletions pandas/tests/extension/decimal/array.py
Original file line number Diff line number Diff line change
Expand Up @@ -138,5 +138,9 @@ def _concat_same_type(cls, to_concat):
return cls(np.concatenate([x._data for x in to_concat]))


def to_decimal(values, context=None):
return DecimalArray([decimal.Decimal(x) for x in values], context=context)


DecimalArray._add_arithmetic_ops()
DecimalArray._add_comparison_ops()
9 changes: 7 additions & 2 deletions pandas/tests/extension/decimal/test_decimal.py
Original file line number Diff line number Diff line change
Expand Up @@ -252,9 +252,14 @@ def test_arith_series_with_array(self, data, all_arithmetic_operators):
context.traps[decimal.DivisionByZero] = divbyzerotrap
context.traps[decimal.InvalidOperation] = invalidoptrap

@pytest.mark.skip(reason="divmod not appropriate for decimal")
def test_divmod(self, data):
pass
s = pd.Series(data, name='name')
a, b = divmod(s, 2)
ea, eb = zip(*(divmod(x, 2) for x in s))
ea = pd.Series(ea, name=s.name, dtype=s.dtype)
eb = pd.Series(eb, name=s.name, dtype=s.dtype)
tm.assert_series_equal(a, ea)
tm.assert_series_equal(b, eb)

def test_error(self):
pass
Expand Down
22 changes: 22 additions & 0 deletions pandas/tests/extension/test_ops.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
import pytest
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 didn't see a good place for generic ops tests. Am I missing someplace?

Copy link
Member

Choose a reason for hiding this comment

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

You mean one that is not supposed to be subclassed by the actual EA implementations?

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 mean things that are testing pandas dispatching, not something in pandas (so not in tests/arrays) and not something that's part of the EA interface (so not part of tests/extension/base).

Copy link
Member

Choose a reason for hiding this comment

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

not something that's part of the EA interface (so not part of tests/extension/base).

OK, but basically we have been testing the ScalarOpsMixin through its use in DecimalArray I think, so you could see this one also as such a test.


from pandas.tests.extension.decimal.array import to_decimal
import pandas.util.testing as tm


@pytest.mark.parametrize("reverse, expected_div, expected_mod", [
(False, [0, 1, 1, 2], [1, 0, 1, 0]),
(True, [2, 1, 0, 0], [0, 0, 2, 2]),
])
def test_divmod(reverse, expected_div, expected_mod):
# https://github.com/pandas-dev/pandas/issues/22930
arr = to_decimal([1, 2, 3, 4])
if reverse:
div, mod = divmod(2, arr)
else:
div, mod = divmod(arr, 2)
expected_div = to_decimal(expected_div)
expected_mod = to_decimal(expected_mod)

tm.assert_extension_array_equal(div, expected_div)
tm.assert_extension_array_equal(mod, expected_mod)