Skip to content

BUG: divmod return type #22932

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 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()
28 changes: 25 additions & 3 deletions pandas/tests/extension/decimal/test_decimal.py
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@

from pandas.tests.extension import base

from .array import DecimalDtype, DecimalArray
from .array import DecimalDtype, DecimalArray, to_decimal


def make_data():
Expand Down Expand Up @@ -244,9 +244,31 @@ 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")
@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_array(self, reverse, expected_div, expected_mod):
# https://github.com/pandas-dev/pandas/issues/22930
Copy link
Member

Choose a reason for hiding this comment

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

Should we try to better indicate that this is an "extra" test (not overriding a base one)?

(not necessarily needs to be solved here, but question is relevant in general, as we sometimes add tests to eg decimal to test specific aspects not covered in the base tests, to clearly see which those tests are. Maybe we would put them outside the class?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Having it off the class makes the most sense. Moved.

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)

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