-
-
Notifications
You must be signed in to change notification settings - Fork 18.4k
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
BUG: divmod return type #22932
Changes from 4 commits
52538fa
c92a4a8
1b4261f
0671e7d
35d4213
7ef697c
c9fe5d3
2247461
a0cd5e7
11a0d93
cc2bfc8
4e9b7f0
95d5cbf
c9d6e89
ec814db
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. is the zip-star necessary? If we get here shouldn't There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. right now > /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 | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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(): | ||
|
@@ -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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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?) There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
|
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.