Skip to content

REGR: diff_2d raising for int8, int16 #39069

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 7 commits into from
Jan 10, 2021
Merged

REGR: diff_2d raising for int8, int16 #39069

merged 7 commits into from
Jan 10, 2021

Conversation

mzeitlin11
Copy link
Member

@mzeitlin11 mzeitlin11 added Algos Non-arithmetic algos: value_counts, factorize, sorting, isin, clip, shift, diff Dtype Conversions Unexpected or buggy dtype conversions Regression Functionality that used to work in a prior pandas version labels Jan 9, 2021
jreback
jreback requested changes Jan 9, 2021
View reviewed changes
pandas/core/algorithms.py Outdated
@@ -1991,7 +1991,11 @@ def diff(arr, n: int, axis: int = 0, stacklevel=3):

elif is_integer_dtype(dtype):
# We have to cast in order to be able to hold np.nan
dtype = np.float64
# int8, int16 are incompatible with float64
Copy link
Contributor

Choose a reason for hiding this comment

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

where exactly is the NotImplmementedError coming from. This should work as-is.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thought this change made sense because of this logic:

pandas/pandas/_libs/algos.pyx

Lines 1270 to 1277 in 66a30bd

# Disable for unsupported dtype combinations,
# see https://github.com/cython/cython/issues/2646
if (out_t is float32_t
and not (diff_t is float32_t or diff_t is int8_t or diff_t is int16_t)):
raise NotImplementedError
elif (out_t is float64_t
and (diff_t is float32_t or diff_t is int8_t or diff_t is int16_t)):
raise NotImplementedError

Was raising in that second branch

Copy link
Contributor

Choose a reason for hiding this comment

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

grr this is super annoying. ok can you add this reference where you are making the code change itself.

Copy link
Contributor

Choose a reason for hiding this comment

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

e.g. here

Copy link
Member Author

Choose a reason for hiding this comment

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

done

jreback
jreback reviewed Jan 9, 2021
View reviewed changes
pandas/tests/test_algos.py
@@ -2409,3 +2409,10 @@ def test_diff_ea_axis(self):
msg = "cannot diff DatetimeArray on axis=1"
with pytest.raises(ValueError, match=msg):
algos.diff(dta, 1, axis=1)

@pytest.mark.parametrize("dtype", ["int8", "int16"])
Copy link
Contributor

Choose a reason for hiding this comment

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

can you also add a replica of the test from the OP in groupby tests. I don't think we actually have any :sigh:

pandas/tests/groupby//test_groupby.py near shift is fine (or actually ok with moving the shift & diff tests out to a new file).

pls add for all dtypes

Copy link
Member Author

Choose a reason for hiding this comment

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

Added test from issue and for other dtypes

@jreback jreback added this to the 1.2.1 milestone Jan 10, 2021
@jreback
Copy link
Contributor

jreback commented Jan 10, 2021

great will merge shortly. the precommit failure fixed elsewhere.

@jreback jreback merged commit 1e900ec into pandas-dev:master Jan 10, 2021
@jreback
Copy link
Contributor

jreback commented Jan 10, 2021

thanks @mzeitlin11

@jreback
Copy link
Contributor

jreback commented Jan 10, 2021

@meeseeksdev backport 1.2.x

@lumberbot-app
Copy link

lumberbot-app bot commented Jan 10, 2021

Owee, I'm MrMeeseeks, Look at me.

There seem to be a conflict, please backport manually. Here are approximate instructions:

  1. Checkout backport branch and update it.
$ git checkout 1.2.x
$ git pull
  1. Cherry pick the first parent branch of the this PR on top of the older branch:
$ git cherry-pick -m1 1e900ecbbad92c5db3755a53c2abb21e146921c5
  1. You will likely have some merge/cherry-pick conflict here, fix them and commit:
$ git commit -am 'Backport PR #39069: REGR: diff_2d raising for int8, int16'
  1. Push to a named branch :
git push YOURFORK 1.2.x:auto-backport-of-pr-39069-on-1.2.x
  1. Create a PR against branch 1.2.x, I would have named this PR:

"Backport PR #39069 on branch 1.2.x"

And apply the correct labels and milestones.

Congratulation you did some good work ! Hopefully your backport PR will be tested by the continuous integration and merged soon!

If these instruction are inaccurate, feel free to suggest an improvement.

@mzeitlin11 mzeitlin11 deleted the regr/diff_2d_low_prec_int branch January 10, 2021 00:45
jreback pushed a commit to jreback/pandas that referenced this pull request Jan 10, 2021
@mzeitlin11 @jreback
Backport PR pandas-dev#39069: REGR: diff_2d raising for int8, int16
bf46b20
jreback pushed a commit to jreback/pandas that referenced this pull request Jan 10, 2021
@mzeitlin11 @jreback
Backport PR pandas-dev#39069: REGR: diff_2d raising for int8, int16
2059a13
jreback pushed a commit that referenced this pull request Jan 10, 2021
@mzeitlin11 @jreback
Backport PR #39069: REGR: diff_2d raising for int8, int16
2ee88b8
luckyvs1 pushed a commit to luckyvs1/pandas that referenced this pull request Jan 20, 2021
@mzeitlin11 @luckyvs1
REGR: diff_2d raising for int8, int16 (pandas-dev#39069)
d6a8c23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jreback jreback jreback approved these changes

Assignees
No one assigned
Labels
Algos Non-arithmetic algos: value_counts, factorize, sorting, isin, clip, shift, diff Dtype Conversions Unexpected or buggy dtype conversions Regression Functionality that used to work in a prior pandas version
Projects
None yet
Milestone
1.2.1
Development

Successfully merging this pull request may close these issues.

BUG: .diff() not working on int8 and int16 dtype
3 participants
@mzeitlin11 @jreback @simonjayhawkins