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

Merged
merged 7 commits into from
Jan 10, 2021
Merged

REGR: diff_2d raising for int8, int16 #39069

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
72cd1ac
REGR: diff_2d low precision int
mzeitlin11 Jan 9, 2021
5c818d8
Add whatsnew
mzeitlin11 Jan 9, 2021
4278126
Use dtype name like other condition
mzeitlin11 Jan 9, 2021
ca5fa87
Address comments
mzeitlin11 Jan 9, 2021
01e1fdd
Add new test file
mzeitlin11 Jan 9, 2021
ae7ed31
Precommit fixup
mzeitlin11 Jan 9, 2021
a015967
Fix whatsnew conflict
mzeitlin11 Jan 9, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions doc/source/whatsnew/v1.2.1.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ Fixed regressions
- Bug in :meth:`read_csv` with ``float_precision="high"`` caused segfault or wrong parsing of long exponent strings. This resulted in a regression in some cases as the default for ``float_precision`` was changed in pandas 1.2.0 (:issue:`38753`)
- Fixed regression in :meth:`Rolling.skew` and :meth:`Rolling.kurt` modifying the object inplace (:issue:`38908`)
- Fixed regression in :meth:`read_csv` and other read functions were the encoding error policy (``errors``) did not default to ``"replace"`` when no encoding was specified (:issue:`38989`)
- Fixed regression in :meth:`DataFrameGroupBy.diff` raising for ``int8`` and ``int16`` columns (:issue:`39050`)

.. ---------------------------------------------------------------------------

Expand Down
6 changes: 5 additions & 1 deletion pandas/core/algorithms.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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

if arr.dtype.name in ["int8", "int16"]:
dtype = np.float32
else:
dtype = np.float64

orig_ndim = arr.ndim
if orig_ndim == 1:
Expand Down
7 changes: 7 additions & 0 deletions pandas/tests/test_algos.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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

def test_diff_low_precision_int(self, dtype):
arr = np.array([0, 1, 1, 0, 0], dtype=dtype)
result = algos.diff(arr, 1)
expected = np.array([np.nan, 1, 0, -1, 0], dtype="float32")
tm.assert_numpy_array_equal(result, expected)