-
-
Notifications
You must be signed in to change notification settings - Fork 18.5k
ENH: Sorting of ExtensionArrays #19957
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.
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
Changes from 9 commits
0ec3600
4707273
b61fb8d
44b6d72
5be3917
e474c20
c2578c3
b73e303
0db9e97
baf624c
ce92f7b
8cbfc36
425fb2a
7bbe796
35a8977
7efece2
5c758aa
d5e8198
c776133
4885245
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1390,17 +1390,24 @@ def check_for_ordered(self, op): | |
"you can use .as_ordered() to change the " | ||
"Categorical to an ordered one\n".format(op=op)) | ||
|
||
def argsort(self, ascending=True, kind='quicksort', *args, **kwargs): | ||
""" | ||
Returns the indices that would sort the Categorical instance if | ||
'sort_values' was called. This function is implemented to provide | ||
compatibility with numpy ndarray objects. | ||
def _values_for_argsort(self): | ||
return self._codes.copy() | ||
|
||
While an ordering is applied to the category values, arg-sorting | ||
in this context refers more to organizing and grouping together | ||
based on matching category values. Thus, this function can be | ||
called on an unordered Categorical instance unlike the functions | ||
'Categorical.min' and 'Categorical.max'. | ||
def argsort(self, *args, **kwargs): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not sure if this changes our opinion on
Changing the Categorical.argsort to accept just |
||
# TODO(PY2): use correct signature | ||
# We have to do *args, **kwargs to avoid a a py2-only signature | ||
# issue since np.argsort differs from argsort. | ||
"""Return the indicies that would sort the Categorical. | ||
|
||
Parameters | ||
---------- | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. do these doc-strings meet the new standards? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 7bbe796 does, aside from examples which isn't really possible. |
||
ascending : bool, default True | ||
Whether the indices should result in an ascending | ||
or descending sort. | ||
kind : {'quicksort', 'mergesort', 'heapsort'}, optional | ||
Sorting algorithm. | ||
args, kwargs: | ||
passed through to :func:`numpy.argsort`. | ||
|
||
Returns | ||
------- | ||
|
@@ -1409,12 +1416,28 @@ def argsort(self, ascending=True, kind='quicksort', *args, **kwargs): | |
See also | ||
-------- | ||
numpy.ndarray.argsort | ||
|
||
Notes | ||
----- | ||
While an ordering is applied to the category values, arg-sorting | ||
in this context refers more to organizing and grouping together | ||
based on matching category values. Thus, this function can be | ||
called on an unordered Categorical instance unlike the functions | ||
'Categorical.min' and 'Categorical.max'. | ||
|
||
Examples | ||
-------- | ||
>>> pd.Categorical(['b', 'b', 'a', 'c']).argsort() | ||
array([2, 0, 1, 3]) | ||
|
||
>>> cat = pd.Categorical(['b', 'b', 'a', 'c'], | ||
... categories=['c', 'b', 'a'], | ||
... ordered=True) | ||
>>> cat.argsort() | ||
array([3, 0, 1, 2]) | ||
""" | ||
ascending = nv.validate_argsort_with_ascending(ascending, args, kwargs) | ||
result = np.argsort(self._codes.copy(), kind=kind, **kwargs) | ||
if not ascending: | ||
result = result[::-1] | ||
return result | ||
# Keep the implementation here just for the docstring. | ||
return super(Categorical, self).argsort(*args, **kwargs) | ||
|
||
def sort_values(self, inplace=False, ascending=True, na_position='last'): | ||
""" Sorts the Categorical by category value returning a new | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why is this needed? shouldn't this one of our myriad of _values methods/properties here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
_ndarray_valaues
seems like more of an internal thing, no? I don't know enough to say whether the_ndarray_values
appropriate for our current uses (mostly indexing IIRC) are also appropriate for argsort.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
my point is what is the point of overriding this specific one? why is not a general purpose EA method/property used here. The proliferation of methods properties is really troublesome.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if someone wants to override argsort great. but providing an indirect mechism is really kludgey.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR is implementing argsort. I could see a similar pattern for factorize.
How so?
What's kudgey about it? I think the common case will be overriding the values provided to
np.argsort
, not overriding the sorting algorithm itself. This is true for Categorical and IPArray, and will be true for Period and probably others.Without
_values_for_argsort
we, and 3rd party libraries, will have duplicate code for validating keyword arguments and the ascending kwarg.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't necessarily want to convert into extension arrays into the same NumPy array used for
np.asarray()
.For example, the IP Address extension array probably wants to convert into numpy object array of IPAddress object. But for sorting, it could just return numpy structured array with a few integer fields, which will be much faster for comparisons than Python IPAddress objects.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed. In the IPArray case, there's a numpy array, so
_values_for_argsort
is just that array.Then we're back in the duplicate code situation. That's OK for the base class, but Categorical, Period, and Interval will end up re-implementing argsort from scratch. With
_values_for_argsort
, it's just a matter of constructing that array (codes, ordinals, concat left & right).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On the name
_values_for_argsort
, it's possible that we'll find other uses for it, in which case we just change the name.Do you know if a simple array appropriate for arg-sorting is also appropriate for factorize, joins, indexing, etc? I'm not sure ahead of time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, we can scratch factorize / groupby off. Categorical defines
_codes_for_groupby
separately from itsndarray_values
(codes)pandas/pandas/core/arrays/categorical.py
Line 638 in 3783ccc
So we can't just use one array for everything.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, so then let's pick a name change
_codes_for_groupby
. in this refactor we want to find other usecases and fix our code now rather than later.something like:
_int_mapping_for_values