Index.intersection changed behavior to sort by default in pandas 0.24

[shoyer]

Prior to #24521 (i.e., for pandas 0.23 and earlier), pandas.Index.intersection would not sort values:

# pandas 0.23.3
In [21]: pd.Index(['c', 'b', 'a']).intersection(['b', 'a'])
Out[21]: Index(['b', 'a'], dtype='object')

Now it does:

In [28]: pd.Index(['c', 'b', 'a']).intersection(['b', 'a'])
Out[28]: Index(['a', 'b'], dtype='object')

This turned up in a failure in xarray's test suite (not a real bug): pydata/xarray#2717

It's fine to change this for consistency, but a deprecation cycle would probably make sense so users aren't surprised by the behavior of their code changing, e.g., we could default to sort=None and issue a FutureWarning for now.

[jreback]

union and difference do sort by default; i suppose intersection was just not clear; in any event the option is now there to control this

[shoyer]

The documented behavior of Index.section was not to sort. To quote the pandas 0.23.x docs:
"This returns a new Index with elements common to the index and other, preserving the order of the calling index."
http://pandas.pydata.org/pandas-docs/version/0.23/generated/pandas.Index.intersection.html

This language has been there since at last pandas 0.20.x, July 2017:
http://pandas.pydata.org/pandas-docs/version/0.20/generated/pandas.Index.intersection.html

[jorisvandenbossche]

We also failed to actually mention this change in the whatsnew or docstring.

I agree that ideally we should have done it with a FutureWarning. Still adding one for 0.24.1 of course changes the behaviour again ..

[TomAugspurger]

I think doing a quick 0.24.1 (within a few days) switching this is the best option. Will put up a PR for discussion.

[TomAugspurger]

But just to be clear, not sorting is better right? Preserving the original order should be the default?

[jorisvandenbossche]

Trying to get an overview of things.

In 0.23.4, we had the following behaviour:

• None of the methods had a sort keyword

• Index.intersection preserved the original order in left and right. Eg:

  In [84]: pd.Index(['a', 'e', 'c']).intersection(['e', 'd', 'c']) 
  Out[84]: Index(['e', 'c'], dtype='object')
  

• Index.union typically sorted, but not in all cases:

  In [86]: pd.Index(['a', 'e', 'c']).union(['a', 'b'])
  Out[86]: Index(['a', 'b', 'c', 'e'], dtype='object')
  

  A case where it does not sort: equal index objects:

  In [88]: pd.Index(['a', 'e', 'c']).union(pd.Index(['a', 'e', 'c']))
  Out[88]: Index(['a', 'e', 'c'], dtype='object')
  

---

In pandas 0.24.0, we added a sort keyword to most of the set-operation methods. First to Index.difference (#22811), later also to union and intersection (#24521).

To match the current behaviour, a default of sort=True was used for union and difference. @reidy-p is that the correct reason? Or were there other arguments?

But, for Index.intersection, a default of sort=True actually changed the behaviour, hence this issue.

---

So we had inconsistent behaviour between intersection and union/difference, and also within one method (sometimes it is not sorted, see example above).
Long term, I think we would ideally get a consistent behaviour. The question is then what the default behaviour should be.

I think there are several reasons for this to be False.Eg that is more consistent with what we did for eg concat? (that will no longer sort by default in the future)

[shoyer]

Long term, I agree that:

1. Index operations should all consistently sort or not sort by default
2. Not sorting by default is probably preferable

Also: changing this behavior immediately in 0.24.0 without a deprecation cycle was not ideal. I think we should revert this change, by setting the default for intersection back to sort=False. Then we can change the default to sort=None (for "legacy behavior" vs "explicitly unsorted") and start a deprecation cycle.

I'm also worried about the new default of sort=True for union. This will almost certainly entail a breaking change for users when we fix union() to actually always sort (e.g., in the case of equal objects). Instead, I think we should make the default sort=None (for "legacy behavior") and raise errors if sort=True is passed but we can't or don't actually sort.

[TomAugspurger]

Then we can change the default to sort=None (for "legacy behavior" vs "explicitly unsorted") and start a deprecation cycle.

What would the benefit of a deprecation cycle be? IIUC the desired long-term behavior for Index.intersection is sort=False. If we release a 0.24.1 with sort=False, then we shouldn't need a deprecation cycle, since every version of pandas except 0.24.0 does the right thing.

I didn't really appreciate the the subtleties of Index.union(sort=True) until now... I agree that passing sort=True should always result in a sorted index or error, but we also don't want to change behavior. Your suggestion of sort=None meaning "legacy" behavior (with all the complexities of maybe not sorting) is good.

errors if sort=True is passed but we can't or don't actually sort.

I think that long-term, once the default for Index.union is sort=False, we can even sort identical objects (rather than raising an error). So we would only raise when the user explicitly requests sort=True and the objects aren't comparable.

[jreback]

@shoyer Note that Index.union() previously and currently defaults to sort=True

[shoyer]

What would the benefit of a deprecation cycle be? IIUC the desired long-term behavior for Index.intersection is sort=False. If we release a 0.24.1 with sort=False, then we shouldn't need a deprecation cycle, since every version of pandas except 0.24.0 does the right thing.

Right, nevermind. We won't need a deprecation cycle if we don't change this :)

[shoyer]

Note that Index.union() previously and currently defaults to sort=True

Not quite :). The previous default of Index.union() is "usually sort". Now we're on the verge of establishing sort=True as also indicating "usually sort".

[jreback]

Not quite :). The previous default of Index.union() is "usually sort". Now we're on the verge of establishing sort=True as also indicating "usually sort".

Not sure what you mean. It previously sorted and still does by default. That is the same. Sure you have the option now NOT to sort.

[jorisvandenbossche]

Not sure what you mean. It previously sorted and still does by default.

@jreback For example, it does not sort (by default, if sort=True) when indices are equal. See my example above (#24959 (comment))

[jreback]

@jorisvandenbossche that is a case w/o distinction. We don't do anything when things are equal in lots of cases, e.g. .reindex(). So this would not change.

[TomAugspurger]

But then we're in the situation @shoyer described in
#24959 (comment), where sort=True means "usually sort". That's bad right?

[jorisvandenbossche]

Indeed, keeping this "sort unless objects are equal" as the default with sort=True, basically means that you are not guaranteed that the result is sorted, even when passing sort=True, which IMO is somewhat unfortunate.

[TomAugspurger]

So, if we agree that sort=True's current behavior of only sometimes sorting is bad, then what do we do?

Proposal: For 0.24.1, change the default to None, which is the old behavior.

sort : bool or None, default None
    Whether to sort the result of the union.
    By default, labels are sorted when

      * `self` is not identical to `other`
      * The items in `self` and `other` can all be compared.

    Specify ``sort=True`` to always attempt to sort the values,
    even if `self` and `other` are identical, or the values cannot
    be compared. If the values cannot be compared, and ``sort=True``,
    a ``TypeError`` is raised.

    Specify ``sort=False`` to not attempt to sort the values.

Later (0.25.0), we deprecate sort=None and say that in the future the default will be sort=False.

My only hesitation with that is it no longer allows the old behavior of sorting iff the values are not identical and comparable. But is that old behavior useful in practice?

[TomAugspurger]

@jreback thoughts on

1. Changing the default Index.difference to sort=False, to be compatible with pre-0.23.4 (
   REGR: Preserve order by default in Index.intersection #24967)
2. Changing the default Index.union to sort=None Index.intersection changed behavior to sort by default in pandas 0.24 #24959 (comment)

2 isn't time-sensitive, since sort=True is backwards compatible with pre-0.23.4. It could be done as part of 0.25.

[jorisvandenbossche]

+1 on Tom's proposal.

I think there is agreement on that we want sort=False eventually as the long-term behaviour? (@jreback is that correct? I assume so based on #24967 (comment))

If we agree on that, I think the most logical thing (most easy to deal with for code depending on it) is reverting the intersection behaviour back to sort=False for 0.24.1, as Tom proposes.

And then actually deprecating sort=True/None for the others is indeed less time sensitive / probably better for 0.25.0.

[jreback]

If you want to revert for 0.24.1 ok I guess.

[TomAugspurger]

Thanks. #24967 implements that, and Joris is +1 already. Any other comments there or good to merge?

[shoyer]

And then actually deprecating sort=True/None for the others is indeed less time sensitive / probably better for 0.25.0.

To be honest, I'm not sure it was a good idea to add this argument at all in its current broken state -- I would consider reverting support for sort=True at all until we can sort consistently. Otherwise we will be establishing "sort sometimes" as expected behavior.

[TomAugspurger]

FWIW, I don't think implementing sort=True means "always sort" will take too long if we prefer to do that for 0.24.1. Then we can change the default to sort=None (legacy behavior) and can deprecate that for sort=False at our leisure.

[TomAugspurger]

Opened #25007 with a POC for changing the default of Index.union(sort=None), roughly implementing
#24959 (comment).

Won't have more time to work on that until tonight.

I'm +0.5 for including that in a 0.24.1. If we want to ensure that sort=True means always sort, we'll need a breaking change from 0.24.0. I think it's best to get that done with before people start relying on sort=True (as the default) heavily.

[jorisvandenbossche]

Sorry, forget that last comment (old text that github cached on reload).

Re-opening, as we still have the discussion / PR on the sort default for the other methods.

[reidy-p]

I'm a bit late coming back to this but want to say thanks to everyone for a very thoughtful discussion and for following up so quickly.

Just to add some more context of where I was coming from. I was originally working on #22811 and found that to get groupby.nth to maintain column order I needed to be able to control the sort behaviour of Index.difference so I added a sort parameter to this method. After this was merged in I realised that the other set operations for Index did not have any sort parameters which is unfortunately a bit inconsistent. So I opened #24471 to propose adding sort parameters to the other set operations for Index. This has been discussed in plenty of other issues that I reference in that issue but has never been implemented.

My plan was to implement the sort parameter for Index and other index types but I felt that it would be far too big to put in one PR so I planned to make several PRs and have a checklist in #24471 to make sure all the issues and corner-cases (such as equal indices) were covered. I agree with @shoyer that it's unfortunate that the parameter is pretty broken at the moment because I didn't get a chance to do as much work on it as I wanted before the release of 0.24.0 and I apologise for any inconvenience. I'd be happy to keep working on the sort parameter to make it work properly once we have any immediate issues with the legacy behaviour fixed.

[TomAugspurger]

No worries @reidy-p, totally understood about getting busy. Index.union sorting by default was a major headache, just need to clean up some of the edge cases.

[TomAugspurger]

Adding this back to 0.24.1 milestone FYI.