REGR: Preserve order by default in Index.intersection
State: closed

Closes https://github.com/pandas-dev/pandas/issues/24959

Labels:
name: API Design
name: Deprecate
description: Functionality to remove in pandas

Comments:
If we do this, we still need to decide the long-term behavior.

I thought that we eventually wanted to make `Index.union()` preserve the original order if possible (`sort=False`), so we would want `Index.intersection` to just have `sort=False` as the default today.
-1 on changing this
it’s already
done and this will make it very confusing

the long term should probably be sort=False
What exactly are you -1 on?
# [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/24967?src=pr&el=h1) Report
> Merging [#24967](https://codecov.io/gh/pandas-dev/pandas/pull/24967?src=pr&el=desc) into [master](https://codecov.io/gh/pandas-dev/pandas/commit/2b16e2e6c5a298396727fc2e66a60edf1eb13bf9?src=pr&el=desc) will **decrease** coverage by `49.49%`.
> The diff coverage is `100%`.

[![Impacted file tree graph](https://codecov.io/gh/pandas-dev/pandas/pull/24967/graphs/tree.svg?width=650&token=eZ4WkYLtcO&height=150&src=pr)](https://codecov.io/gh/pandas-dev/pandas/pull/24967?src=pr&el=tree)

```diff
@@            Coverage Diff             @@
##           master   #24967      +/-   ##
==========================================
- Coverage   92.38%   42.88%   -49.5%     
==========================================
  Files         166      166              
  Lines       52398    52398              
==========================================
- Hits        48406    22472   -25934     
- Misses       3992    29926   +25934
```

| Flag | Coverage Δ | |
|---|---|---|
| #multiple | `?` | |
| #single | `42.88% <100%> (-0.01%)` | :arrow_down: |

| [Impacted Files](https://codecov.io/gh/pandas-dev/pandas/pull/24967?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pandas/core/indexes/multi.py](https://codecov.io/gh/pandas-dev/pandas/pull/24967/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvaW5kZXhlcy9tdWx0aS5weQ==) | `34.33% <100%> (-61.26%)` | :arrow_down: |
| [pandas/core/indexes/interval.py](https://codecov.io/gh/pandas-dev/pandas/pull/24967/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvaW5kZXhlcy9pbnRlcnZhbC5weQ==) | `35.14% <100%> (-60.11%)` | :arrow_down: |
| [pandas/core/indexes/datetimes.py](https://codecov.io/gh/pandas-dev/pandas/pull/24967/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvaW5kZXhlcy9kYXRldGltZXMucHk=) | `48.46% <100%> (-47.82%)` | :arrow_down: |
| [pandas/core/indexes/base.py](https://codecov.io/gh/pandas-dev/pandas/pull/24967/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvaW5kZXhlcy9iYXNlLnB5) | `56.27% <100%> (-40.06%)` | :arrow_down: |
| [pandas/core/indexes/range.py](https://codecov.io/gh/pandas-dev/pandas/pull/24967/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvaW5kZXhlcy9yYW5nZS5weQ==) | `52.78% <100%> (-44.58%)` | :arrow_down: |
| [pandas/io/formats/latex.py](https://codecov.io/gh/pandas-dev/pandas/pull/24967/diff?src=pr&el=tree#diff-cGFuZGFzL2lvL2Zvcm1hdHMvbGF0ZXgucHk=) | `0% <0%> (-100%)` | :arrow_down: |
| [pandas/core/categorical.py](https://codecov.io/gh/pandas-dev/pandas/pull/24967/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvY2F0ZWdvcmljYWwucHk=) | `0% <0%> (-100%)` | :arrow_down: |
| [pandas/io/sas/sas\_constants.py](https://codecov.io/gh/pandas-dev/pandas/pull/24967/diff?src=pr&el=tree#diff-cGFuZGFzL2lvL3Nhcy9zYXNfY29uc3RhbnRzLnB5) | `0% <0%> (-100%)` | :arrow_down: |
| [pandas/tseries/plotting.py](https://codecov.io/gh/pandas-dev/pandas/pull/24967/diff?src=pr&el=tree#diff-cGFuZGFzL3RzZXJpZXMvcGxvdHRpbmcucHk=) | `0% <0%> (-100%)` | :arrow_down: |
| [pandas/tseries/converter.py](https://codecov.io/gh/pandas-dev/pandas/pull/24967/diff?src=pr&el=tree#diff-cGFuZGFzL3RzZXJpZXMvY29udmVydGVyLnB5) | `0% <0%> (-100%)` | :arrow_down: |
| ... and [128 more](https://codecov.io/gh/pandas-dev/pandas/pull/24967/diff?src=pr&el=tree-more) | |

------

[Continue to review full report at Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/24967?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/24967?src=pr&el=footer). Last update [2b16e2e...17d0f92](https://codecov.io/gh/pandas-dev/pandas/pull/24967?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

# [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/24967?src=pr&el=h1) Report
> Merging [#24967](https://codecov.io/gh/pandas-dev/pandas/pull/24967?src=pr&el=desc) into [master](https://codecov.io/gh/pandas-dev/pandas/commit/bf693ffb520da3b315737a8d52595646afa74d63?src=pr&el=desc) will **not change** coverage.
> The diff coverage is `100%`.

[![Impacted file tree graph](https://codecov.io/gh/pandas-dev/pandas/pull/24967/graphs/tree.svg?width=650&token=eZ4WkYLtcO&height=150&src=pr)](https://codecov.io/gh/pandas-dev/pandas/pull/24967?src=pr&el=tree)

```diff
@@           Coverage Diff           @@
##           master   #24967   +/-   ##
=======================================
  Coverage   92.38%   92.38%           
=======================================
  Files         166      166           
  Lines       52400    52400           
=======================================
  Hits        48409    48409           
  Misses       3991     3991
```

| Flag | Coverage Δ | |
|---|---|---|
| #multiple | `90.8% <100%> (ø)` | :arrow_up: |
| #single | `42.87% <100%> (-0.01%)` | :arrow_down: |

| [Impacted Files](https://codecov.io/gh/pandas-dev/pandas/pull/24967?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pandas/core/indexes/multi.py](https://codecov.io/gh/pandas-dev/pandas/pull/24967/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvaW5kZXhlcy9tdWx0aS5weQ==) | `95.59% <100%> (ø)` | :arrow_up: |
| [pandas/core/indexes/interval.py](https://codecov.io/gh/pandas-dev/pandas/pull/24967/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvaW5kZXhlcy9pbnRlcnZhbC5weQ==) | `95.25% <100%> (ø)` | :arrow_up: |
| [pandas/core/indexes/datetimes.py](https://codecov.io/gh/pandas-dev/pandas/pull/24967/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvaW5kZXhlcy9kYXRldGltZXMucHk=) | `96.27% <100%> (ø)` | :arrow_up: |
| [pandas/core/indexes/base.py](https://codecov.io/gh/pandas-dev/pandas/pull/24967/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvaW5kZXhlcy9iYXNlLnB5) | `96.32% <100%> (ø)` | :arrow_up: |
| [pandas/core/indexes/range.py](https://codecov.io/gh/pandas-dev/pandas/pull/24967/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvaW5kZXhlcy9yYW5nZS5weQ==) | `97.36% <100%> (ø)` | :arrow_up: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/24967?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/24967?src=pr&el=footer). Last update [bf693ff...61dbd38](https://codecov.io/gh/pandas-dev/pandas/pull/24967?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

> What exactly are you -1 on?

The change is done and its consistent with other set ops, which sort.  Sure it broke, but reverting makes this more confusing. 

In 
https://github.com/pandas-dev/pandas/pull/24967#issuecomment-457960350 you said you want long-term sort=False. Is that correct or a typo?

I'm not sure why we would want to break the long-standing behavior, deprecate the new behavior, and then eventually change back to the old behavior.

@shoyer what would be easier for xarray's here?
> In #24967 (comment) you said you want long-term sort=False. Is that correct or a typo?

yes, but for ALL set ops. Having one different is pretty odd.
Maybe? But having this one be different is clearly preferable to breaking from the previous behavior, right?
> Maybe? But having this one be different is clearly preferable to breaking from the previous behavior, right?

no I don't think so. Its unfortunate that we didn't have any testing around this, but I don't see a reason to create confusion . what's done is done.
K. We'll need input from others then; I think getting us back to the pre-0.24.0 behavior is a good idea.

I think @jorisvandenbossche also prefers changing it back in 0.24.1.
Since needing input from others is explicitly mentioned I'm dropping a comment even though I've never contributed to pandas. The default `sort=True` option broke some unit tests for me this morning, and is what led me to this issue. 

I would expect the default behaviour to be `sort=False` because:
1. creating/reading a dataframe or resetting the index doesn't sort automatically, so I would expect to receive an unsorted index when operating on it
2. returning just the index series does not return a sorted index so I would expect to receive an unsorted index when operating on it

Also, I think reasonable default behaviour moving forward is more important than any confusion that might arise from 0.24.0 vs. 0.24.1. 

Thanks for all the work on pandas, we rely on it extensively. Hope my comments are not intrusive
Thanks @samspills.

> Also, I think reasonable default behaviour moving forward is more important than any confusion that might arise from 0.24.0 vs. 0.24.1.

Just to be clear, the long-term goal is to have `sort=False` for all of our index setops. The question is how do we get there from 0.24.0.

1. Implement a deprecation warning for the current behavior (sort=True), and eventually change the default to `sort=False`.
2. Do a quick "break" from 0.24.0 to restore the 0.23.4 and earlier behavior with `sort=False` as the default. No deprecation cycle needed.

IMO, 2 is easier on users and downstream libraries. For people who never want intersection to sort, it's easiest to require `pandas != 0.24.0`, which has only been out for a few days.

If we go with option 1, all downstream users wanting to avoid sorting and support multiple versions of pandas will need to update all their calls to Index.intersection.
I also just commented about it on the issue: https://github.com/pandas-dev/pandas/issues/24959#issuecomment-458214689 Shall we continue the discussion there or here?

@samspills thanks for chiming in!
If we want `sort=False` as the long term behaviour for all set ops, then I agree with Tom: let's just quickly revert `Index.intersection` to have `sort=False` as default. That will make the life of people that depend on it easier.
From a user perspective, I think it would be preferable to blacklist a specific pandas version in our setup.py than to require either `<0.24` or `>=0.24.0`+specify sort behavour explicitly 😄 
> 1. Implement a deprecation warning for the current behavior (sort=True), and eventually change the default to sort=False.

yes I think we should do this, but for all setops


> 2. Do a quick "break" from 0.24.0 to restore the 0.23.4 and earlier behavior with sort=False as the default. No deprecation cycle needed.

I don't think we should do this. what's done is done.
@jreback I think you’re the only one who prefers 1 (others may speak up). Are you OK with deferring to the majority on this one?
@jreback latest discussion/proposal is actually on the issue: https://github.com/pandas-dev/pandas/issues/24959#issuecomment-458264154
> > Implement a deprecation warning for the current behavior (sort=True), and eventually change the default to sort=False.
>
> yes I think we should do this, but for all setops

That is what Tom proposed in https://github.com/pandas-dev/pandas/issues/24959#issuecomment-458264154

To clarify 
https://github.com/pandas-dev/pandas/pull/24967#issuecomment-458552145, I think everyone is in favor of eventually getting to `sort=False` as the default, the question is how we get there. Most people seem to prefer `Index.intersection()` to have `sort=False` now, to match the behavior of pandas<0.24.0, and the future desired behavior.
I guess this is ok to push.
@TomAugspurger Thanks!