-
-
Notifications
You must be signed in to change notification settings - Fork 18.4k
[BUG] Series.diff was always setting the dtype to object #30894
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
Conversation
7a9a833
to
f217fdb
Compare
@jreback @jbrockmendel @WillAyd thanks for your quick reviews, I've updated the PR accordingly |
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.
Thanks for the 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.
Isn't the correct fix to to dispatch diff
to the EA, rather than trying to fix the type after the fact? Or can we perhaps write a version of diff
in terms of take
and __sub__
?
this is actually a good idea would be simple to write as a generic EA |
@TomAugspurger @jreback I don't understand. Currently, we have
In the case of
then
is already converting the values to Object before even reaching
So should Anyway, I realise that there's a somewhat tight deadline to get v1.0.0 out, and I'm happy to keep tackling this issue, but if you think it is (currently) beyond my capabilities, I'm happy for someone else to take over :) |
We would need to not call that, at least for EAs, but probably for all dtypes, before passing it off. I can probably take a look tomorrow. We have a week or two for 1.0.0 final. |
bdc1883
to
6ad19ca
Compare
c4b5cb7
to
ac7bb30
Compare
@TomAugspurger OK thanks - I might have figured out what you were suggesting anyway, have pushed another commit |
See #31025 for an alternative which allows EAs to define how the operation should be done. |
I think we'll proceed with #31025. Apologies for the duplicated effort @MarcoGorelli. |
It's OK, it was a good learning experience to attempt this and then read through your PR! |
black pandas
git diff upstream/master -u -- "*.py" | flake8 --diff