Skip to content

BUG: Always cast to Categorical in lexsort_indexer #36385

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
Sep 17, 2020
Merged

BUG: Always cast to Categorical in lexsort_indexer #36385

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
1df4dd7
BUG: Always cast to Categorical in lexsort_indexer
dsaxton Sep 15, 2020
9652042
Nit
dsaxton Sep 15, 2020
d45ae2d
Edit test
dsaxton Sep 15, 2020
ab13b98
Diff order
dsaxton Sep 15, 2020
595791b
Drop import
dsaxton Sep 15, 2020
578bb3d
Param
dsaxton Sep 15, 2020
37d73e3
Merge remote-tracking branch 'upstream/master' into categorical-sort-…
dsaxton Sep 15, 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
1 change: 1 addition & 0 deletions doc/source/whatsnew/v1.1.3.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ Bug fixes
- Bug in :func:`read_spss` where passing a ``pathlib.Path`` as ``path`` would raise a ``TypeError`` (:issue:`33666`)
- Bug in :meth:`Series.str.startswith` and :meth:`Series.str.endswith` with ``category`` dtype not propagating ``na`` parameter (:issue:`36241`)
- Bug in :class:`Series` constructor where integer overflow would occur for sufficiently large scalar inputs when an index was provided (:issue:`36291`)
- Bug in :meth:`DataFrame.sort_values` raising an ``AttributeError`` when sorting on a key that casts column to categorical dtype (:issue:`36383`)
- Bug in :meth:`DataFrame.stack` raising a ``ValueError`` when stacking :class:`MultiIndex` columns based on position when the levels had duplicate names (:issue:`36353`)

.. ---------------------------------------------------------------------------
Expand Down
9 changes: 1 addition & 8 deletions pandas/core/sorting.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@
from pandas.core.dtypes.common import (
ensure_int64,
ensure_platform_int,
is_categorical_dtype,
is_extension_array_dtype,
)
from pandas.core.dtypes.generic import ABCMultiIndex
Expand Down Expand Up @@ -294,13 +293,7 @@ def lexsort_indexer(
keys = [ensure_key_mapped(k, key) for k in keys]

for k, order in zip(keys, orders):
# we are already a Categorical
if is_categorical_dtype(k):
cat = k

# create the Categorical
else:
cat = Categorical(k, ordered=True)
cat = Categorical(k, ordered=True)

if na_position not in ["last", "first"]:
raise ValueError(f"invalid na_position: {na_position}")
Expand Down
20 changes: 20 additions & 0 deletions pandas/tests/frame/methods/test_sort_values.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -691,3 +691,23 @@ def test_sort_values_key_dict_axis(self):
result = df.sort_values(1, key=lambda col: -col, axis=1)
expected = df.loc[:, ::-1]
tm.assert_frame_equal(result, expected)

@pytest.mark.parametrize("ordered", [True, False])
def test_sort_values_key_casts_to_categorical(self, ordered):
# https://github.com/pandas-dev/pandas/issues/36383
categories = ["c", "b", "a"]
df = pd.DataFrame({"x": [1, 1, 1], "y": ["a", "b", "c"]})

def sorter(key):
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 try when the categorical is ordered=False as well (parameterize)

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah this is broken actually. Will need to be more careful above.

Copy link
Member Author

Choose a reason for hiding this comment

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

Or maybe my expectation is off about how this should behave when the categorical is unordered. This is odd (it seems to respect the order in which categories are given even when ordered=False):

[ins] In [1]: import pandas as pd
         ...:
         ...:
         ...: values = ["a", "b", "c"]
         ...:
         ...: cat = pd.Categorical(values, categories=["a", "b", "c"], ordered=False)
         ...: print(cat.sort_values())
         ...:
         ...: cat = pd.Categorical(values, categories=["c", "b", "a"], ordered=False)
         ...: print(cat.sort_values())
         ...:
         ...: print(pd.__version__)
         ...:
['a', 'b', 'c']
Categories (3, object): ['a', 'b', 'c']
['c', 'b', 'a']
Categories (3, object): ['c', 'b', 'a']
1.2.0.dev0+390.g595791b6f.dirty

Maybe sorting an unordered categorical should actually be raising.

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe sorting an unordered categorical should actually be raising.

had this discussion with @jorisvandenbossche a while back......

yeah sorting just gives back the same ordering as the categories that you have, they just don't mean anything.

so we do allow it.

Copy link
Contributor

Choose a reason for hiding this comment

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

what I think is broken is actually this

In [184]: pd.Categorical(pd.Categorical(values, categories=["a", "b", "c"], ordered=False), ordered=True)                                                       
Out[184]: 
[a, b, c]
Categories (3, object): [a < b < c]

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this should raise, though we do allow this via .set_ordered() so maybe its ok

cc @TomAugspurger

Copy link
Contributor

Choose a reason for hiding this comment

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

since we aren't actually testing this likely i think prob ok to merge this and open an issue for discussion.

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 would almost expect sorting an unordered categorical to simply return the original array (since maybe you could argue it's already "trivially ordered" in some sense) if it weren't to raise

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually, R does the same thing as pandas interestingly enough:

> x <- factor(c("a", "b", "c", "a"), levels = c("c", "b", "a"), ordered = FALSE)
> x
[1] a b c a
Levels: c b a
> sort(x)
[1] c b a a
Levels: c b a

I guess because it's easiest just to always sort by the underlying codes.

Copy link
Member

Choose a reason for hiding this comment

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

I would almost expect sorting an unordered categorical to simply return the original array (since maybe you could argue it's already "trivially ordered" in some sense)

@dsaxton strings also don't necessarily have a meaningfull order, but we still sort them lexicographically. In the same way, we still sort an unordered categorical, using the order of the categories (which is the same as lexicographically sorted in most cases, unless you specified the categories manually in a certain order).

There are lots of reasons to allow sorting for an "unordered" categorical. One example is to get a deterministic order of your values, which can be useful regardless of the order of the categories having a meaning or not.

if key.name == "y":
return pd.Series(
pd.Categorical(key, categories=categories, ordered=ordered)
)
return key

result = df.sort_values(by=["x", "y"], key=sorter)
expected = pd.DataFrame(
{"x": [1, 1, 1], "y": ["c", "b", "a"]}, index=pd.Index([2, 1, 0])
)

tm.assert_frame_equal(result, expected)