Skip to content
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

REGR: fix numpy accumulate ufuncs for DataFrame #39260

Merged
merged 4 commits into from
Jan 19, 2021
Merged

REGR: fix numpy accumulate ufuncs for DataFrame #39260

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
d285e5a
REGR: fix numpy accumulate ufuncs for DataFrame
jorisvandenbossche Jan 18, 2021
668a9d2
add example + clarifying notes
jorisvandenbossche Jan 19, 2021
d28e32a
Merge remote-tracking branch 'upstream/master' into array-ufunc-frame…
jorisvandenbossche Jan 19, 2021
13dcd5c
add reference to numpy docs
jorisvandenbossche Jan 19, 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 @@ -32,6 +32,7 @@ Fixed regressions
- Fixed regression in :meth:`DataFrame.replace` raising ``ValueError`` when :class:`DataFrame` has dtype ``bytes`` (:issue:`38900`)
- Fixed regression in :meth:`Series.fillna` that raised ``RecursionError`` with ``datetime64[ns, UTC]`` dtype (:issue:`38851`)
- Fixed regression in comparisons between ``NaT`` and ``datetime.date`` objects incorrectly returning ``True`` (:issue:`39151`)
- Fixed regression in calling NumPy :func:`~numpy.ufunc.accumulate` ufuncs on DataFrames, e.g. ``np.maximum.accumulate(df)`` (:issue:`39259`)
- Fixed regression in repr of float-like strings of an ``object`` dtype having trailing 0's truncated after the decimal (:issue:`38708`)
- Fixed regression that raised ``AttributeError`` with PyArrow versions [0.16.0, 1.0.0) (:issue:`38801`)
- Fixed regression in :func:`pandas.testing.assert_frame_equal` raising ``TypeError`` with ``check_like=True`` when :class:`Index` or columns have mixed dtype (:issue:`39168`)
Expand Down
10 changes: 8 additions & 2 deletions pandas/core/arraylike.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -274,8 +274,14 @@ def reconstruct(result):
result = getattr(ufunc, method)(*inputs, **kwargs)
else:
# ufunc(dataframe)
mgr = inputs[0]._mgr
result = mgr.apply(getattr(ufunc, method))
if method == "__call__":
# for np.<ufunc>(..) calls
mgr = inputs[0]._mgr
result = mgr.apply(getattr(ufunc, method))
else:
# otherwise specific ufunc methods (eg np.<ufunc>.accumulate(..))
# Those can have an axis keyword and thus can't be called block-by-block
result = getattr(ufunc, method)(np.asarray(inputs[0]), **kwargs)

if ufunc.nout > 1: # type: ignore[attr-defined]
result = tuple(reconstruct(x) for x in result)
Expand Down
21 changes: 21 additions & 0 deletions pandas/tests/frame/test_ufunc.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,6 +117,27 @@ def test_binary_frame_series_raises():
np.logaddexp(df["A"], df)


def test_unary_accumulate_axis():
# https://github.com/pandas-dev/pandas/issues/39259
df = pd.DataFrame({"a": [1, 3, 2, 4]})
result = np.maximum.accumulate(df)
expected = pd.DataFrame({"a": [1, 3, 3, 4]})
tm.assert_frame_equal(result, expected)

df = pd.DataFrame({"a": [1, 3, 2, 4], "b": [0.1, 4.0, 3.0, 2.0]})
result = np.maximum.accumulate(df)
# in theory could preserve int dtype for default axis=0
expected = pd.DataFrame({"a": [1.0, 3.0, 3.0, 4.0], "b": [0.1, 4.0, 4.0, 4.0]})
tm.assert_frame_equal(result, expected)
Comment on lines +129 to +131
Copy link
Contributor

Choose a reason for hiding this comment

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

Mmm, I'd like this to be more than "in theory". I'd consider this a buggy test, since things should be done blockwise for axis=0.

Can you you change the test case to have just floats or just ints (even if you have to manually split it for test coverage?).

Copy link
Member Author

Choose a reason for hiding this comment

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

I specifically used two dtypes to have two blocks to ensure we handle this case correctly for axis=1 (which can never be done clockwise)

Just above there is already a case with only ints that preserves the int dtype.

Copy link
Member Author

Choose a reason for hiding this comment

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

Note that before pandas 1.2.0, this also didn't preserve the dtypes per column, and 1.2.0 itself didn't calculate a proper result (so I would call this PR a strict improvement ;))

Copy link
Member

Choose a reason for hiding this comment

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

agreed re preserving for axis=0. couldn't we still use mgr.apply in that case?

Copy link
Member Author

Choose a reason for hiding this comment

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

To be clear, I could certainly detect the special case of accumulate with axis=0, and then pass axis=1 to the blocks, but:

1) that requires special case code like this in array_ufunc:

else:
    # the ufunc(dataframe) case
    ...
    elif method == "accumulate" and ("axis" not in kwargs or ("axis" in kwargs and kwargs["axis"] == 0)):
        # swap axis for the transposed Block values
        kwargs["axis"] = 1
        result = mgr.apply(getattr(ufunc, method), **kwargs)

2) that requires Block.apply to be "aware" of axis. Currently it simply passes through keywords, but in this case it would need to interpret axis differently depending on whether its values are stored as 2D or 1D (and I know we already need to take this axis swapping into account in many places, eg with NDFrame._get_block_manager_axis(axis), and in the internals as well, but that's typically when axis is a keyword of our own, and not a user-specified kwarg of a generic applied function). So I certainly could add an ExtensionBlock.apply override to take this into account. But we could also decide to leave this as is for now.

Copy link
Member

Choose a reason for hiding this comment

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

but in this case it would need to interpret axis differently depending on whether its values are stored as 2D or 1D

Once more with feeling: this wouldn't be an issue with 2D EAs.

Copy link
Member Author

Choose a reason for hiding this comment

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

Once more with feeling: this wouldn't be an issue with 2D EAs.

And I can also say: this wouldn't be an issue with only 1D arrays ..

Copy link
Member Author

Choose a reason for hiding this comment

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

@TomAugspurger @jbrockmendel I opened #39275 to keep track of the fact that this can be improved to preserve dtypes


result = np.maximum.accumulate(df, axis=0)
tm.assert_frame_equal(result, expected)

result = np.maximum.accumulate(df, axis=1)
expected = pd.DataFrame({"a": [1.0, 3.0, 2.0, 4.0], "b": [1.0, 4.0, 3.0, 4.0]})
tm.assert_frame_equal(result, expected)


def test_frame_outer_deprecated():
df = pd.DataFrame({"A": [1, 2]})
with tm.assert_produces_warning(FutureWarning):
Expand Down