BUG: divmod return type #22932

TomAugspurger · 2018-10-01T21:52:22Z

pep8speaks · 2018-10-01T21:52:25Z

Hello @TomAugspurger! Thanks for submitting the PR.

There are no PEP8 issues in the file pandas/core/arrays/base.py !
There are no PEP8 issues in the file pandas/tests/extension/decimal/array.py !
There are no PEP8 issues in the file pandas/tests/extension/test_ops.py !

TomAugspurger · 2018-10-01T21:56:49Z

pandas/tests/extension/test_ops.py

@@ -0,0 +1,22 @@
+import pytest


I didn't see a good place for generic ops tests. Am I missing someplace?

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

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).

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.

codecov · 2018-10-02T03:14:12Z

Codecov Report

Merging #22932 into master will decrease coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master   #22932      +/-   ##
==========================================
- Coverage    92.2%   92.19%   -0.01%     
==========================================
  Files         169      169              
  Lines       50822    50837      +15     
==========================================
+ Hits        46858    46870      +12     
- Misses       3964     3967       +3

Flag	Coverage Δ
#multiple	`90.61% <100%> (-0.01%)`	⬇️
#single	`42.34% <0%> (-0.02%)`	⬇️

Impacted Files	Coverage Δ
pandas/core/arrays/base.py	`95.89% <100%> (+0.17%)`	⬆️
pandas/core/reshape/merge.py	`93.89% <0%> (-0.26%)`	⬇️
pandas/util/testing.py	`86.18% <0%> (-0.05%)`	⬇️
pandas/core/strings.py	`98.63% <0%> (ø)`	⬆️
pandas/core/dtypes/cast.py	`88.58% <0%> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fe67b94...ec814db. Read the comment docs.

TomAugspurger · 2018-10-02T11:54:05Z

FYI, this, and the following are holding up PeriodArray a bit, so if I could get a quick +/- 1 on

it'd be appreciated. (cc @jbrockmendel for this one in particular.)

jorisvandenbossche · 2018-10-02T14:26:54Z

doc/source/extending.rst

+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.


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

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.

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

jorisvandenbossche · 2018-10-02T14:31:56Z

pandas/tests/extension/test_ops.py

@@ -0,0 +1,22 @@
+import pytest


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

jorisvandenbossche · 2018-10-02T15:01:29Z

FYI, this, and the following are holding up PeriodArray a bit

Just a question, I would expect that Period does not need the ScalarOpsMixin? (it has its own vectorized implementations?)

TomAugspurger · 2018-10-02T15:07:05Z

Hmm, yes you're right. I'll look on my other branch to see if / why I was hitting this...

jbrockmendel · 2018-10-02T22:42:19Z

pandas/core/arrays/base.py

-                    pass
+                if op.__name__ in {'divmod', 'rdivmod'}:
+                    try:
+                        a, b = zip(*res)


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

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')))

jbrockmendel · 2018-10-02T22:46:55Z

This may be over-optimizing for code-sharing, but it might be worth updating ops._construct_result and ops._construct_divmod_result for compat with EA (instead of just Series) and use the same pattern as _arith_method_SERIES

construct_result = (_construct_divmod_result
                    if op is divmod else _construct_result)

Now that I look at it, it looks like ops.dispatch_to_extension_op hard-codes _construct_result so will fail on divmod/rdivmod

jbrockmendel · 2018-10-02T22:49:39Z

Can you add a test that does divmod(Series[EA], Series[EA])? (or divmod(Series[EA], EA), or ...). I think that will necessitate a small fix in ops.dispatch_to_extension_op, but that should be fixed at the same time as this. (or heck, add the test(s) and xfail it and I'll fix it in a follow-up)

Pending that: LGTM.

TomAugspurger · 2018-10-03T01:49:51Z

I'll make those fixes to ops.dispatch_to_extension_op here. Will see what we can share.

TomAugspurger · 2018-10-03T18:33:19Z

Added a test with divmod(Series[EA], EA]).

Now that I look at it, it looks like ops.dispatch_to_extension_op hard-codes _construct_result so will fail on divmod/rdivmod

It seems correct on master, maybe someone (probably you) fixed it in the meantime?

but it might be worth updating ops._construct_result and ops._construct_divmod_result for compat with EA

Perhaps leave for #22974, as I think there are some details to work out. I haven't put any tests in place for divmod(EA, Series[EA]) yet, since I think master is incorrect. Right now that's a tuple of (EA, EA), but should maybe be a tuple of (Series[EA], Series[EA]).

jbrockmendel · 2018-10-03T20:59:16Z

Perhaps leave for #22974, as I think there are some details to work out

Sounds good.

TomAugspurger · 2018-10-04T13:35:19Z

Updated.

Deduplicated the try / except, now that we do it in 3 places instead of 2
changed some tests that were unexpectedly passing, when they should be skips.

jorisvandenbossche · 2018-10-05T11:25:39Z

pandas/tests/extension/decimal/test_decimal.py

+        (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


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?)

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

jreback · 2018-10-07T22:57:24Z

pandas/core/arrays/base.py

+                    res = np.asarray(arr)
+                return res
+
+            if op.__name__ in {'divmod', 'rdivmod'}:


@jbrockmendel can't we use dispatch_to_extension_op here to avoid duplication of code?

I asked something similar a few days ago. If Tom says it isn't feasible, I believe him.

We are inside the extension array here, so it would also be strange to use (which doesn't prevent that both could share a helper function, if that would be appropriate).
But here we need to construct the divmod correctly, while dispatch_to_extension_op should assume this is already done correctly by the EA

which doesn't prevent that both could share a helper function,

Right. This is possible, if people want it. I'll push up a commit with some kind of do_extension_op that both of these call to so people can take a look.

Ah, now that I take a look it's not so straightforward. The two are similar but just slightly different in enough places that they wouldn't benefit from sharing code really.

The unboxing of values. dispatch_to_extension_op knows that at least one of the two is a Series[EA]. _binop knows that self is an EA.

The op: dispatch_to_extension_op dispatches, _binop is defining it in a list comprehension

The re-boxing: _binop has the whole maybe re-constructing _from_seqence that the dispatch_to_extension_op doesn't have to worry about at all.

They do not do "conceptually exactly the same thing". Paraphrasing myself from above:

The dispatch function calls the EA to perform an operation, the above code is the EA doing the operation.

Why would those two different things necessarily need to live in the same place / code path?

Of course, we could still move the whole EA._create_method to ops.py (which would indeed be similar as functions like add_flex_arithmetic_methods in ops.py that is used in series.py to add methods to Series). But this is then not related to the change in this PR, and should be left for another issue/PR to discuss (personally I don't think that would be an improvement).

I would see if its is possible to integrate these rather than adding a bunch of new code.

Well, and both Tom and me who have looked into the code, say: we don't think it is possible.

@TomAugspurger I saw your comment. I also @jorisvandenbossche comments. I have not looked at this in detail, nor do I have time to. My point is that this instantly creates technical debt no matter how you slice it.

It may require some reorganization to integrate this, and I appreciate that. So happy to defer this, maybe @jbrockmendel has more insight.

I'll be doing a sparse-de-duplication PR following #22880, can take a fresh look at this then. In the interim, I wouldn't let this issue hold up this PR.

My point is that this instantly creates technical debt no matter how you slice it.

It really doesn't. They're doing two different things.

This PR is fixing a bug, not doing a refactor of how the ops on EA's are implemented** . If somebody want to look into that, it should be done in a separate PR anyway. So merging.

** and I fully acknowledge that sometimes, to properly fix a bug, you also need to refactor otherwise you just keep adding hacks. However, I don't think that is the case here, see all the comments above.

TomAugspurger · 2018-10-08T12:13:34Z

Good to merge this then?

BUG: divmod return type

52538fa

TomAugspurger added Numeric Operations Arithmetic, Comparison, and Logical operations ExtensionArray Extending pandas with custom dtypes or arrays. labels Oct 1, 2018

TomAugspurger added this to the 0.24.0 milestone Oct 1, 2018

Update old test

c92a4a8

TomAugspurger commented Oct 1, 2018

View reviewed changes

TomAugspurger mentioned this pull request Oct 2, 2018

S3 parquet failures on Travis #22934

Closed

4 tasks

jorisvandenbossche reviewed Oct 2, 2018

View reviewed changes

TomAugspurger added 2 commits October 2, 2018 10:58

Merge remote-tracking branch 'upstream/master' into ea-divmod

1b4261f

Fixup

0671e7d

TomAugspurger mentioned this pull request Oct 2, 2018

Catch Exception in combine #22936

Merged

jbrockmendel reviewed Oct 2, 2018

View reviewed changes

TomAugspurger added 4 commits October 3, 2018 13:11

Merge remote-tracking branch 'upstream/master' into ea-divmod

35d4213

Use super

7ef697c

make strict

c9fe5d3

Test op(Series[EA], EA])

2247461

TomAugspurger added 2 commits October 3, 2018 14:25

TypeError for Series

a0cd5e7

typerror

11a0d93

TomAugspurger added 2 commits October 4, 2018 08:15

Merge remote-tracking branch 'upstream/master' into ea-divmod

cc2bfc8

doc update

4e9b7f0

TomAugspurger added 2 commits October 4, 2018 08:22

typo, import

95d5cbf

xpass -> skip

c9d6e89

jorisvandenbossche approved these changes Oct 5, 2018

View reviewed changes

Move

ec814db

jreback requested changes Oct 7, 2018

View reviewed changes

jorisvandenbossche merged commit e510b1a into pandas-dev:master Oct 8, 2018

TomAugspurger deleted the ea-divmod branch October 8, 2018 13:56

tm9k1 pushed a commit to tm9k1/pandas that referenced this pull request Nov 19, 2018

BUG: divmod return type (pandas-dev#22932)

21d0830

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

BUG: divmod return type #22932

BUG: divmod return type #22932

TomAugspurger commented Oct 1, 2018

pep8speaks commented Oct 1, 2018

TomAugspurger Oct 1, 2018

jorisvandenbossche Oct 2, 2018

TomAugspurger Oct 2, 2018

jorisvandenbossche Oct 2, 2018

codecov bot commented Oct 2, 2018 •

edited

TomAugspurger commented Oct 2, 2018

jorisvandenbossche Oct 2, 2018

TomAugspurger Oct 2, 2018

TomAugspurger Oct 2, 2018

jorisvandenbossche Oct 2, 2018

jorisvandenbossche commented Oct 2, 2018

TomAugspurger commented Oct 2, 2018

jbrockmendel Oct 2, 2018

TomAugspurger Oct 3, 2018

jbrockmendel commented Oct 2, 2018

jbrockmendel commented Oct 2, 2018

TomAugspurger commented Oct 3, 2018

TomAugspurger commented Oct 3, 2018

jbrockmendel commented Oct 3, 2018

TomAugspurger commented Oct 4, 2018

jorisvandenbossche Oct 5, 2018

TomAugspurger Oct 5, 2018

jreback Oct 7, 2018

jbrockmendel Oct 8, 2018

jorisvandenbossche Oct 8, 2018

TomAugspurger Oct 8, 2018

TomAugspurger Oct 8, 2018

jorisvandenbossche Oct 8, 2018

jreback Oct 8, 2018

jbrockmendel Oct 8, 2018

TomAugspurger Oct 8, 2018

jorisvandenbossche Oct 8, 2018

TomAugspurger commented Oct 8, 2018

BUG: divmod return type #22932

BUG: divmod return type #22932

Conversation

TomAugspurger commented Oct 1, 2018

pep8speaks commented Oct 1, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Oct 2, 2018 • edited

Codecov Report

TomAugspurger commented Oct 2, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jorisvandenbossche commented Oct 2, 2018

TomAugspurger commented Oct 2, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbrockmendel commented Oct 2, 2018

jbrockmendel commented Oct 2, 2018

TomAugspurger commented Oct 3, 2018

TomAugspurger commented Oct 3, 2018

jbrockmendel commented Oct 3, 2018

TomAugspurger commented Oct 4, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TomAugspurger commented Oct 8, 2018

codecov bot commented Oct 2, 2018 •

edited