-
-
Notifications
You must be signed in to change notification settings - Fork 18.4k
ENH partial sorting for mi in sortlevel #6135
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
Conversation
Would it be more general to overload |
I was just concerned about keeping old (strange?) behavior, I don't follow how to do this with types, but definitely the kwargs are inelegant. |
What I meant was you can express the same operation and more |
That's a nice idea. The issue is the current behaviour is to sort all when you pass a single level e.g. sortlevel('A')... maybe this shouldn't be the default and we could do something more logical (like what you suggest)? Actually this current behaviour is a little deranged... is sortlevels the only way to sort a mi (sort doesn't work!)... |
You'll have to give examples, I'm not sure what you mean by deranged. You probably tried the following already:
That sorts by one and only one level, and it looks stable wrt to the other levels doing so. What are you after, preferably with explicit example? |
"That sorts by one and only one level," here's an example, let's reverse:
|
now I see what you mean. |
This is kindof documented in the DataFrame.sortlevel, but tbh this is more of an observation than a spec:
... :s |
Yeah, it's a tossup on which docstring is more abstruse, You could have If you want to sort on multiple single levels, just do multiple invocations. |
But now I understand why you chose to have the default True keyword. makes sense. |
@hayd It's maybe also an option to fix this in |
@jorisvandenbossche I wasn't sure if this was part of @jtratner's work on refactoring index - and making the common methods. +1 a level kwarg makes sense! I was confused about what sort_index actually does, I thought it would sort by the index, but it appears to accept columns... i.e is just sort with an axis arg? |
hows this coming? |
@jreback Is the remaining thing here to add a level (and sort_remaining?) arg to sort and sort_index ? |
I think this is fine...wouldn't really change sort/sort_index ATM. ping me to look at this in a day or 2 |
ping |
this was nice.... any other possibilites besides |
Not sure, happy to change, what's your issue with sort_remaining? (sort_remaining_levels ?) |
just sounds odd - though can't think of anything better you are defaulting this to true right? |
yeah, defaults to true so doesn't break backwards compat... (though perhaps we will later) |
maybe I think it's a perf issue to completely sort |
@jreback shall we get this in? |
looks fine release note / v0.14.0 mention merge when ready I don't think docs are really necessary as the do string covers |
looks fine....just check release notes / v0.14 |
@@ -2627,7 +2627,8 @@ def trans(v): | |||
else: | |||
return self.take(indexer, axis=axis, convert=False, is_copy=False) | |||
|
|||
def sortlevel(self, level=0, axis=0, ascending=True, inplace=False): | |||
def sortlevel(self, level=0, axis=0, ascending=True, | |||
inplace=False, sort_remaining=True): | |||
""" | |||
Sort multilevel index by chosen axis and primary level. Data will be |
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.
Would this be more clear as:
Lexicographically sort index of dataframe on specified axis, starting with the specified level and then sorting by other levels in the order they're defined on the multilevel index (
sort_remaining
can optionally disable sorting on other levels).
It's not perfect, but maybe clearer?
👍 from me as well. In addition to the minor suggestion on the docstring, should we add a warning about the performance hit from non-lexsorted |
ping! |
ping |
@hayd ping....need to get this in ASAP |
ping! |
ENH partial sorting for mi in sortlevel
fixes #3984.
Still need to tweak the docstrings and stuff. But I think this is working.
cc @jtratner