API: Change default for Index.union sort
State: closed

Closes https://github.com/pandas-dev/pandas/issues/24959

Haven't done MultiIndex yet, just opening for discussion on *if* we should do this for 0.24.1.

Labels:
name: Indexing
description: Related to indexing on series/frames, not to indexes themselves
name: API Design

Comments:
> We should then do the same for intersection and difference?

Not intersection. Probably difference and symmetric_difference.

> This looks good to me.

Just to confirm for posterity, good for 0.24.1? If so I'll get back to it in ~4 hours to finish things off.
> Not intersection.

`intersection` also has the try/except on sorting. So although the default there is `sort=False`, we still might want to have "correct" behaviour for `sort=True/None` ?

> Just to confirm for posterity, good for 0.24.1? If so I'll get back to it in ~4 hours to finish things off.

For me that would be the best option, but we might want confirmation of others.
> intersection also has the try/except on sorting.

Ah yes, I was thinking ahead to the deprecation.

But, do we think that `.intersection(sort=None)` is useful? Or will we eventually deprecate it anyway?
> intersection also has the try/except on sorting. So although the default there is sort=False, we still might want to have "correct" behaviour for sort=True/None ?

I don't see the try/except around sorting for Index.intersection.

```python
In [11]: a.intersection(a[::-1], sort=True)
Traceback (most recent call last):
  File "<ipython-input-11-2e1c550543d3>", line 1, in <module>
    a.intersection(a[::-1], sort=True)
  File "/Users/taugspurger/sandbox/pandas/pandas/core/indexes/base.py", line 2431, in intersection
    taken = sorting.safe_sort(taken.values)
  File "/Users/taugspurger/sandbox/pandas/pandas/core/sorting.py", line 459, in safe_sort
    ordered = sort_mixed(values)
  File "/Users/taugspurger/sandbox/pandas/pandas/core/sorting.py", line 452, in sort_mixed
    nums = np.sort(values[~str_pos])
  File "/Users/taugspurger/Envs/pandas-dev/lib/python3.7/site-packages/numpy/core/fromnumeric.py", line 934, in sort
    a.sort(axis=axis, kind=kind, order=order)
  File "pandas/_libs/tslibs/timestamps.pyx", line 258, in pandas._libs.tslibs.timestamps._Timestamp.__richcmp__
    raise TypeError('Cannot compare type %r with type %r' %
TypeError: Cannot compare type 'Timestamp' with type 'int'


In [12]: a.intersection(a[::-1], sort=False)
Out[12]: Index([1, 2000-01-01 00:00:00], dtype='object')
```

I do see the special casing when the indexes are equal

```
In [16]: a = pd.Index(['b', 'a'])

In [17]: a.intersection(a, sort=True)
Out[17]: Index(['b', 'a'], dtype='object')
```
Following up on 
https://github.com/pandas-dev/pandas/pull/25007#issuecomment-458790111, IMO `Index.intersection(sort=None)` is not needed. I believe the two behaviors enabled by `sort=None` are

1. No sort when `idx.equals(idx)`.
2. No exception when elements of `self & other` are not comparable.

The first role is served equally well by `idx.intersection(other, sort=False)`


```python
In [5]: a = pd.Index(['b', 'a'])

In [6]: a.intersection(a, sort=False)
Out[6]: Index(['b', 'a'], dtype='object')
```

The second, I don't think we should be *adding* that behavior where it wasn't previously. And since Index.intersection didn't previously sort, we don't need to add it.
# [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/25007?src=pr&el=h1) Report
> Merging [#25007](https://codecov.io/gh/pandas-dev/pandas/pull/25007?src=pr&el=desc) into [master](https://codecov.io/gh/pandas-dev/pandas/commit/ece58cb39f2afa6dbc0c64f701847f7efd718a58?src=pr&el=desc) will **decrease** coverage by `49.5%`.
> The diff coverage is `41.66%`.

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

```diff
@@             Coverage Diff             @@
##           master   #25007       +/-   ##
===========================================
- Coverage   92.38%   42.88%   -49.51%     
===========================================
  Files         166      166               
  Lines       52401    52409        +8     
===========================================
- Hits        48410    22474    -25936     
- Misses       3991    29935    +25944
```

| Flag | Coverage Δ | |
|---|---|---|
| #multiple | `?` | |
| #single | `42.88% <41.66%> (ø)` | :arrow_up: |

| [Impacted Files](https://codecov.io/gh/pandas-dev/pandas/pull/25007?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pandas/core/indexes/base.py](https://codecov.io/gh/pandas-dev/pandas/pull/25007/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvaW5kZXhlcy9iYXNlLnB5) | `56.35% <41.66%> (-39.98%)` | :arrow_down: |
| [pandas/io/formats/latex.py](https://codecov.io/gh/pandas-dev/pandas/pull/25007/diff?src=pr&el=tree#diff-cGFuZGFzL2lvL2Zvcm1hdHMvbGF0ZXgucHk=) | `0% <0%> (-100%)` | :arrow_down: |
| [pandas/core/categorical.py](https://codecov.io/gh/pandas-dev/pandas/pull/25007/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/25007/diff?src=pr&el=tree#diff-cGFuZGFzL2lvL3Nhcy9zYXNfY29uc3RhbnRzLnB5) | `0% <0%> (-100%)` | :arrow_down: |
| [pandas/tseries/plotting.py](https://codecov.io/gh/pandas-dev/pandas/pull/25007/diff?src=pr&el=tree#diff-cGFuZGFzL3RzZXJpZXMvcGxvdHRpbmcucHk=) | `0% <0%> (-100%)` | :arrow_down: |
| [pandas/tseries/converter.py](https://codecov.io/gh/pandas-dev/pandas/pull/25007/diff?src=pr&el=tree#diff-cGFuZGFzL3RzZXJpZXMvY29udmVydGVyLnB5) | `0% <0%> (-100%)` | :arrow_down: |
| [pandas/io/formats/html.py](https://codecov.io/gh/pandas-dev/pandas/pull/25007/diff?src=pr&el=tree#diff-cGFuZGFzL2lvL2Zvcm1hdHMvaHRtbC5weQ==) | `0% <0%> (-99.35%)` | :arrow_down: |
| [pandas/core/groupby/categorical.py](https://codecov.io/gh/pandas-dev/pandas/pull/25007/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvZ3JvdXBieS9jYXRlZ29yaWNhbC5weQ==) | `0% <0%> (-95.46%)` | :arrow_down: |
| [pandas/io/sas/sas7bdat.py](https://codecov.io/gh/pandas-dev/pandas/pull/25007/diff?src=pr&el=tree#diff-cGFuZGFzL2lvL3Nhcy9zYXM3YmRhdC5weQ==) | `0% <0%> (-91.17%)` | :arrow_down: |
| [pandas/io/sas/sas\_xport.py](https://codecov.io/gh/pandas-dev/pandas/pull/25007/diff?src=pr&el=tree#diff-cGFuZGFzL2lvL3Nhcy9zYXNfeHBvcnQucHk=) | `0% <0%> (-90.15%)` | :arrow_down: |
| ... and [124 more](https://codecov.io/gh/pandas-dev/pandas/pull/25007/diff?src=pr&el=tree-more) | |

------

[Continue to review full report at Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/25007?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/25007?src=pr&el=footer). Last update [ece58cb...2a2de25](https://codecov.io/gh/pandas-dev/pandas/pull/25007?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/25007?src=pr&el=h1) Report
> Merging [#25007](https://codecov.io/gh/pandas-dev/pandas/pull/25007?src=pr&el=desc) into [master](https://codecov.io/gh/pandas-dev/pandas/commit/149138ef008a26bca04f4b698e15ff8fd3ce4663?src=pr&el=desc) will **increase** coverage by `<.01%`.
> The diff coverage is `100%`.

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

```diff
@@            Coverage Diff             @@
##           master   #25007      +/-   ##
==========================================
+ Coverage   92.37%   92.38%   +<.01%     
==========================================
  Files         166      166              
  Lines       52397    52416      +19     
==========================================
+ Hits        48404    48425      +21     
+ Misses       3993     3991       -2
```

| Flag | Coverage Δ | |
|---|---|---|
| #multiple | `90.81% <100%> (ø)` | :arrow_up: |
| #single | `42.86% <28.57%> (-0.02%)` | :arrow_down: |

| [Impacted Files](https://codecov.io/gh/pandas-dev/pandas/pull/25007?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pandas/core/indexes/multi.py](https://codecov.io/gh/pandas-dev/pandas/pull/25007/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvaW5kZXhlcy9tdWx0aS5weQ==) | `95.6% <100%> (+0.01%)` | :arrow_up: |
| [pandas/core/indexes/base.py](https://codecov.io/gh/pandas-dev/pandas/pull/25007/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvaW5kZXhlcy9iYXNlLnB5) | `96.34% <100%> (+0.02%)` | :arrow_up: |
| [pandas/core/frame.py](https://codecov.io/gh/pandas-dev/pandas/pull/25007/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvZnJhbWUucHk=) | `96.92% <0%> (ø)` | :arrow_up: |
| [pandas/core/tools/numeric.py](https://codecov.io/gh/pandas-dev/pandas/pull/25007/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvdG9vbHMvbnVtZXJpYy5weQ==) | `100% <0%> (ø)` | :arrow_up: |
| [pandas/core/internals/concat.py](https://codecov.io/gh/pandas-dev/pandas/pull/25007/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvaW50ZXJuYWxzL2NvbmNhdC5weQ==) | `96.45% <0%> (ø)` | :arrow_up: |
| [pandas/core/internals/blocks.py](https://codecov.io/gh/pandas-dev/pandas/pull/25007/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvaW50ZXJuYWxzL2Jsb2Nrcy5weQ==) | `94.26% <0%> (+0.08%)` | :arrow_up: |
| [pandas/util/testing.py](https://codecov.io/gh/pandas-dev/pandas/pull/25007/diff?src=pr&el=tree#diff-cGFuZGFzL3V0aWwvdGVzdGluZy5weQ==) | `88.13% <0%> (+0.09%)` | :arrow_up: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/25007?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/25007?src=pr&el=footer). Last update [149138e...b15dc7e](https://codecov.io/gh/pandas-dev/pandas/pull/25007?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

> Following up on
> [#25007 (comment)](https://github.com/pandas-dev/pandas/pull/25007#issuecomment-458790111), IMO `Index.intersection(sort=None)` is not needed.

Agreed!
> I don't see the try/except around sorting for Index.intersection.

Sorry, not sure what I was looking at .. :) It does have the "directly return if equals" behaviour that needs to handle `sort`, but that you already did.

Fully agreed a `sort=None` with new behaviour is not needed for `intersection` !
> We do NOT do this elsewhere, e.g. .reindex, so this is extra useless sorting.

@jreback can you clarify this comparison to reindex? Reindexing does not involve any sorting?
@jorisvandenbossche ``.reindex`` does not reindex if the other is identical (or empty). This is the same exact situation
> .reindex does not reindex if the other is identical (or empty). This is the same exact situation

Yes, but `reindex` has no `sort` keyword, and does not sort in general. So here it's a different situation IMO. 
Once the default would be `sort=False`, then `Index.union` will also not do anything by default if the other is identical.
> Yes, but reindex has no sort keyword, and does not sort in general. So here it's a different situation IMO.
Once the default would be sort=False, then Index.union will also not do anything by default if the other is identical.

not at all. this is the same. changing semantics like this is simply not warranted.
Changes like this need to sit in master. I am -1 on doing this for 0.24.x at all. There is no reason to change at the last minute like this. 
@jreback I would ask again that you defer to others on this one. I'm pretty sure everyone else thinks these two changes are good, and that 0.24.1 should be released quickly with them.
@jorisvandenbossche can you confirm that `symmetric_difference` already follows the new rules of `sort=True` means sort?

```python
In [14]: idx = pd.Index(['b', 'a', 'c'])

In [15]: idx.symmetric_difference([])
Out[15]: Index(['a', 'b', 'c'], dtype='object')

In [16]: pd.Index([]).symmetric_difference(idx)
Out[16]: Index(['a', 'b', 'c'], dtype='object')

In [17]: idx.symmetric_difference(idx)
Out[17]: Index([], dtype='object')
```

It's good that we'll be consistent across all the methods.
> @jreback I would ask again that you defer to others on this one. I'm pretty sure everyone else thinks these two changes are good, and that 0.24.1 should be released quickly with them.

this is way too fast. I am ok with what is in 0.24.1, but see no reason rush this. 
Are you going to block a release? @jorisvandenbossche, @shoyer, and I judge that the changes here are best to get out sooner rather than later.

Stepping back a bit, I don't think these are *large* change right, in the sense that they're like to introduce new bugs? What's the concern that this is "too fast"?
@jreback for the main reason to consider this for 0.24.1 is that there is no change in default behaviour. 
It is true we are adding new behaviour (behind the new keyword `sort`), which ideally can live in master a bit longer. 
An alternative would be to only change the default of `sort=True` to `sort=None` here for 0.24.1, and add the new behaviour of always sorting for `sort=True` in master / for 0.25. But then the question is: what to do with `sort=True` in 0.24.1? IMO we should then disallow it (eg raise a NotImplementedError), which basically means postponing the `sort=True` to 0.25.0 (with `sort=None` as the current default of sometimes sorting still available in 0.24.1 of course). Which also feels a bit stupid to me, hence why I am in favor of doing this for 0.24.1.
> Stepping back a bit, I don't think these are large change right, in the sense that they're like to introduce new bugs? What's the concern that this is "too fast"?

The point of a 0.24.1 release is to fix regressions. I assert this is changing things, hence it should not be included. If you want to preserve the existing API, meaning ``sort=True`` means exactly what it means now then ok. but if you are actually changing it to mean something else (this PR); then I am -1 on it.

I feel this change is simply rushed and should simply live in master. We should be conservative on 'patching' things as they can then also lead to more issues. This is not conservative but very rushed.
>  If you want to preserve the existing API, meaning sort=True means exactly what it means now then ok. but if you are actually changing it to mean something else (this PR); then I am -1 on it.

Are you also -1 on changing the default value of `sort` from True to None? (to preserve `sort=True` for different behaviour in the future) 
And again, with that there would be no change in default *behaviour* (but that this also the case for the current PR)
>  We should be conservative on 'patching' things as they can then also lead to more issues. This is not conservative but very rushed.

There's a tension though: I think we *really* don't want to establish `sort=True` as "sometimes sort", right? So the longer we sit with the 0.24.0 behavior, the harder it is for users in the future / writing code that needs to be compatible across versions.
> There's a tension though: I think we really don't want to establish sort=True as "sometimes sort", 

so what I would do here is maybe add our usual keyword ``errors=raise|warn|None``.

But to be honest I am not sure this is a big deal, we have been issuing a warning if it cannot sort since the beginning of time. What is the motivation for actually changing this now?

I am ok with ``sort=None`` if the ultimate goal is a deprecation so we can reach ``sort=False`` as a default.
> What is the motivation for actually changing this now?

We didn't previously have a `sort` keyword.

> I am ok with sort=None if the ultimate goal is a deprecation so we can reach sort=False as a default.

That is the goal. +1 then?
@TomAugspurger it's still labeled WIP, but I think it is actually ready to be merged?
> can you confirm that symmetric_difference already follows the new rules of sort=True means sort?

The examples you show regarding empty and identical are indeed already correct, but, both `difference` and `symmetric_difference` have the try/except on sorting, so there default is also more a kind of `sort=None` (although not the exactly the same as for union, as here if the result is identical to the caller, it is still sorted).

```
In [47]: idx = pd.Index([1, pd.Timestamp('2000'), 2])                                                                                                                                                               

In [48]: idx.symmetric_difference([])                                                                                                                                                                               
Out[48]: Index([1, 2000-01-01 00:00:00, 2], dtype='object')

In [49]: idx.symmetric_difference([], sort=True)                                                                                                                                                                    
Out[49]: Index([1, 2000-01-01 00:00:00, 2], dtype='object')

In [50]: idx.symmetric_difference([], sort=False)                                                                                                                                                                   
Out[50]: Index([1, 2000-01-01 00:00:00, 2], dtype='object')
```
Right, forgot about the sorting. I'll fix that up now and remove the WIP
label.

On Thu, Jan 31, 2019 at 7:26 AM Joris Van den Bossche <
notifications@github.com> wrote:

> can you confirm that symmetric_difference already follows the new rules of
> sort=True means sort?
>
> The examples you show regarding empty and identical are indeed already
> correct, but, both difference and symmetric_difference have the
> try/except on sorting, so there default is also more a kind of sort=None
> (although not the exactly the same as for union, as here if the result is
> identical to the caller, it is still sorted).
>
> In [47]: idx = pd.Index([1, pd.Timestamp('2000'), 2])
>
> In [48]: idx.symmetric_difference([])
> Out[48]: Index([1, 2000-01-01 00:00:00, 2], dtype='object')
>
> In [49]: idx.symmetric_difference([], sort=True)
> Out[49]: Index([1, 2000-01-01 00:00:00, 2], dtype='object')
>
> In [50]: idx.symmetric_difference([], sort=False)
> Out[50]: Index([1, 2000-01-01 00:00:00, 2], dtype='object')
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub
> <https://github.com/pandas-dev/pandas/pull/25007#issuecomment-459343173>,
> or mute the thread
> <https://github.com/notifications/unsubscribe-auth/ABQHIh5j00wvcCbtZj6OxgzmYk2wD8D8ks5vIu8dgaJpZM4aYnPM>
> .
>

Did the same change for symmdiff, so that `sort=True` means sort.
> Did the same change for symmdiff, so that sort=True means sort.

The same also needs to be done for `difference` ?
Sorry, yes...