Skip to content

BUG: Don't cast nullable Boolean to float in groupby #33089

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 18 commits into from
Apr 7, 2020
Merged

BUG: Don't cast nullable Boolean to float in groupby #33089

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
5f8477f
Don't check isinstance
dsaxton Mar 28, 2020
359231c
Fix categorical test
dsaxton Mar 28, 2020
5311de9
Take out len
dsaxton Mar 28, 2020
0aa91f6
Special case categorical
dsaxton Mar 28, 2020
8546e29
Tests
dsaxton Mar 28, 2020
cf29e31
Revert "Fix categorical test"
dsaxton Mar 28, 2020
62de835
Note
dsaxton Mar 28, 2020
89505ac
Hack test
dsaxton Mar 28, 2020
a426f9d
Black
dsaxton Mar 28, 2020
85fe250
Add tests
dsaxton Mar 28, 2020
d55a74f
Include dict case
dsaxton Mar 28, 2020
c7e38b0
Issue numbers
dsaxton Mar 28, 2020
58b2ba4
Merge remote-tracking branch 'upstream/master' into result-casting
dsaxton Mar 28, 2020
a4e10b6
Merge remote-tracking branch 'upstream/master' into result-casting
dsaxton Mar 29, 2020
fc25876
Move note
dsaxton Mar 29, 2020
db4fb5e
Update tests
dsaxton Mar 29, 2020
34cb8d6
Merge remote-tracking branch 'upstream/master' into result-casting
dsaxton Mar 30, 2020
e55a0d4
Merge remote-tracking branch 'upstream/master' into result-casting
dsaxton Apr 7, 2020
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
2 changes: 2 additions & 0 deletions doc/source/whatsnew/v1.1.0.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -444,6 +444,8 @@ Groupby/resample/rolling
- Bug in :meth:`DataFrameGroupby.transform` produces incorrect result with transformation functions (:issue:`30918`)
- Bug in :meth:`GroupBy.count` causes segmentation fault when grouped-by column contains NaNs (:issue:`32841`)
- Bug in :meth:`DataFrame.groupby` and :meth:`Series.groupby` produces inconsistent type when aggregating Boolean series (:issue:`32894`)
- Bug in :meth:`SeriesGroupBy.first`, :meth:`SeriesGroupBy.last`, :meth:`SeriesGroupBy.min`, and :meth:`SeriesGroupBy.max` returning floats when applied to nullable Booleans (:issue:`33071`)
- Bug in :meth:`DataFrameGroupBy.agg` with dictionary input losing ``ExtensionArray`` dtypes (:issue:`32194`)
- Bug in :meth:`DataFrame.resample` where an ``AmbiguousTimeError`` would be raised when the resulting timezone aware :class:`DatetimeIndex` had a DST transition at midnight (:issue:`25758`)

Reshaping
Expand Down
16 changes: 10 additions & 6 deletions pandas/core/dtypes/cast.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@
ensure_str,
is_bool,
is_bool_dtype,
is_categorical_dtype,
is_complex,
is_complex_dtype,
is_datetime64_dtype,
Expand Down Expand Up @@ -279,12 +280,15 @@ def maybe_cast_result(result, obj: "Series", numeric_only: bool = False, how: st
dtype = maybe_cast_result_dtype(dtype, how)

if not is_scalar(result):
if is_extension_array_dtype(dtype) and dtype.kind != "M":
# The result may be of any type, cast back to original
# type if it's compatible.
if len(result) and isinstance(result[0], dtype.type):
Copy link
Member Author

Choose a reason for hiding this comment

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

This check also blocks things like the conversion of float array to nullable integer which was a problem here as well: #32914

cls = dtype.construct_array_type()
result = maybe_cast_to_extension_array(cls, result, dtype=dtype)
if (
Copy link
Contributor

Choose a reason for hiding this comment

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

again instead of expanding this check, i would completely remove it; it be encompassed in maybe_cast_result_type, which is the point of that function

Copy link
Member Author

@dsaxton dsaxton Mar 29, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I'm not sure if we can completely move this logic into maybe_cast_result_dtype; e.g., for something like agg(pd.Series.nunique) performed on a categorical, we end up with integer counts with an original dtype of categorical, but we don't actually know "how" we got there, so we can't say that the dtype should still be integer based on the kind of operation that was performed.

The datetime check seems to be another issue with not being able to introspect a user-provided function like agg(lambda g: g.iloc[0].year) and know that the output should still be an int and not a datetime.

Copy link
Member

Choose a reason for hiding this comment

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

this is tagged as 1.0.4 at the moment just in case we do another patch release. If so, the changes in this PR should be kept to a minimum?

is_extension_array_dtype(dtype)
and not is_categorical_dtype(dtype)
and dtype.kind != "M"
):
# We have to special case categorical so as not to upcast
# things like counts back to categorical
cls = dtype.construct_array_type()
result = maybe_cast_to_extension_array(cls, result, dtype=dtype)

elif numeric_only and is_numeric_dtype(dtype) or not numeric_only:
result = maybe_downcast_to_dtype(result, dtype)
Expand Down
26 changes: 26 additions & 0 deletions pandas/tests/groupby/test_nth.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -384,6 +384,32 @@ def test_first_last_tz_multi_column(method, ts, alpha):
tm.assert_frame_equal(result, expected)


@pytest.mark.parametrize(
"values",
[
pd.array([True, False], dtype="boolean"),
pd.array([1, 2], dtype="Int64"),
pd.to_datetime(["2020-01-01", "2020-02-01"]),
pd.to_timedelta([1, 2], unit="D"),
],
)
@pytest.mark.parametrize("function", ["first", "last", "min", "max"])
def test_first_last_extension_array_keeps_dtype(values, function):
# https://github.com/pandas-dev/pandas/issues/33071
# https://github.com/pandas-dev/pandas/issues/32194
df = DataFrame({"a": [1, 2], "b": values})
grouped = df.groupby("a")
idx = Index([1, 2], name="a")
expected_series = Series(values, name="b", index=idx)
expected_frame = DataFrame({"b": values}, index=idx)

result_series = getattr(grouped["b"], function)()
tm.assert_series_equal(result_series, expected_series)

result_frame = grouped.agg({"b": function})
tm.assert_frame_equal(result_frame, expected_frame)


def test_nth_multi_index_as_expected():
# PR 9090, related to issue 8979
# test nth on MultiIndex
Expand Down
4 changes: 1 addition & 3 deletions pandas/tests/resample/test_datetime_index.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -122,9 +122,7 @@ def test_resample_integerarray():

result = ts.resample("3T").mean()
expected = Series(
[1, 4, 7],
index=pd.date_range("1/1/2000", periods=3, freq="3T"),
dtype="float64",
[1, 4, 7], index=pd.date_range("1/1/2000", periods=3, freq="3T"), dtype="Int64",
)
tm.assert_series_equal(result, expected)

Expand Down