REGR: Preserve order by default in Index.intersection

[TomAugspurger]

Closes #24959

[TomAugspurger]

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.

[jreback]

-1 on changing this
it’s already
done and this will make it very confusing

the long term should probably be sort=False

[TomAugspurger]

What exactly are you -1 on?

[codecov]

Codecov Report

Merging #24967 into master will decrease coverage by 49.49%.
The diff coverage is 100%.

@@            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%)	⬇️

Impacted Files	Coverage Δ
pandas/core/indexes/multi.py	34.33% <100%> (-61.26%)	⬇️
pandas/core/indexes/interval.py	35.14% <100%> (-60.11%)	⬇️
pandas/core/indexes/datetimes.py	48.46% <100%> (-47.82%)	⬇️
pandas/core/indexes/base.py	56.27% <100%> (-40.06%)	⬇️
pandas/core/indexes/range.py	52.78% <100%> (-44.58%)	⬇️
pandas/io/formats/latex.py	0% <0%> (-100%)	⬇️
pandas/core/categorical.py	0% <0%> (-100%)	⬇️
pandas/io/sas/sas_constants.py	0% <0%> (-100%)	⬇️
pandas/tseries/plotting.py	0% <0%> (-100%)	⬇️
pandas/tseries/converter.py	0% <0%> (-100%)	⬇️
... and 128 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2b16e2e...17d0f92. Read the comment docs.

[codecov]

Codecov Report

Merging #24967 into master will not change coverage.
The diff coverage is 100%.

@@           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%> (ø)	⬆️
#single	42.87% <100%> (-0.01%)	⬇️

Impacted Files	Coverage Δ
pandas/core/indexes/multi.py	95.59% <100%> (ø)	⬆️
pandas/core/indexes/interval.py	95.25% <100%> (ø)	⬆️
pandas/core/indexes/datetimes.py	96.27% <100%> (ø)	⬆️
pandas/core/indexes/base.py	96.32% <100%> (ø)	⬆️
pandas/core/indexes/range.py	97.36% <100%> (ø)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bf693ff...61dbd38. Read the comment docs.

[jorisvandenbossche]

Suggested change

	Sort the resulting MultiIndex if possible
	Sort the resulting index if possible

[jreback]

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.

[TomAugspurger]

In
#24967 (comment) 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?

[jreback]

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.

[TomAugspurger]

Maybe? But having this one be different is clearly preferable to breaking from the previous behavior, right?

[jreback]

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.

[TomAugspurger]

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.

[samspills]

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

[TomAugspurger]

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.

[jorisvandenbossche]

I also just commented about it on the issue: #24959 (comment) Shall we continue the discussion there or here?

@samspills thanks for chiming in!

[jorisvandenbossche]

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.

[samspills]

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 😄

[jreback]

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.

[TomAugspurger]

@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?

[jorisvandenbossche]

@jreback latest discussion/proposal is actually on the issue: #24959 (comment)

[jorisvandenbossche]

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 #24959 (comment)

[TomAugspurger]

To clarify
#24967 (comment), 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.

[jreback]

I guess this is ok to push.

[jorisvandenbossche]

@TomAugspurger Thanks!