API: Change default for Index.union sort #25007

TomAugspurger · 2019-01-29T18:02:12Z

Haven't done MultiIndex yet, just opening for discussion on if we should do this for 0.24.1.

shoyer · 2019-01-29T18:40:31Z

pandas/core/indexes/base.py


            .. versionadded:: 0.24.0

+            .. versionchanged:: 0.24.0


Should be "0.24.1"

jorisvandenbossche

This looks good to me. We should then do the same for intersection and difference?

TomAugspurger · 2019-01-29T21:50:17Z

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.

jorisvandenbossche · 2019-01-29T21:53:35Z

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.

TomAugspurger · 2019-01-29T21:57:52Z

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?

jreback · 2019-01-29T21:59:43Z

pandas/core/indexes/base.py


            .. versionadded:: 0.24.0

+            .. versionchanged:: 0.24.0
+
+               Changed the default `sort` to None, matching the


why is this being changed? this is certainly not a regression at all. This was the default behavior.

To be clear: no behaviour is changed. It was indeed the default, it stays the default. It's only the value that encodes the default that is changed (True -> None), so that True can mean something else (=always sort).

ok, maybe it should be more clear in the doc-string

TomAugspurger · 2019-01-30T02:42:14Z

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.

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')

TomAugspurger · 2019-01-30T02:57:24Z

Following up on
#25007 (comment), IMO Index.intersection(sort=None) is not needed. I believe the two behaviors enabled by sort=None are

No sort when idx.equals(idx).
No exception when elements of self & other are not comparable.

The first role is served equally well by idx.intersection(other, sort=False)

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 · 2019-01-30T03:14:10Z

Codecov Report

Merging #25007 into master will decrease coverage by 49.5%.
The diff coverage is 41.66%.

@@             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%> (ø)`	⬆️

Impacted Files	Coverage Δ
pandas/core/indexes/base.py	`56.35% <41.66%> (-39.98%)`	⬇️
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%)`	⬇️
pandas/io/formats/html.py	`0% <0%> (-99.35%)`	⬇️
pandas/core/groupby/categorical.py	`0% <0%> (-95.46%)`	⬇️
pandas/io/sas/sas7bdat.py	`0% <0%> (-91.17%)`	⬇️
pandas/io/sas/sas_xport.py	`0% <0%> (-90.15%)`	⬇️
... and 124 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 ece58cb...2a2de25. Read the comment docs.

codecov · 2019-01-30T03:14:10Z

Codecov Report

Merging #25007 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            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%> (ø)`	⬆️
#single	`42.86% <28.57%> (-0.02%)`	⬇️

Impacted Files	Coverage Δ
pandas/core/indexes/multi.py	`95.6% <100%> (+0.01%)`	⬆️
pandas/core/indexes/base.py	`96.34% <100%> (+0.02%)`	⬆️
pandas/core/frame.py	`96.92% <0%> (ø)`	⬆️
pandas/core/tools/numeric.py	`100% <0%> (ø)`	⬆️
pandas/core/internals/concat.py	`96.45% <0%> (ø)`	⬆️
pandas/core/internals/blocks.py	`94.26% <0%> (+0.08%)`	⬆️
pandas/util/testing.py	`88.13% <0%> (+0.09%)`	⬆️

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 149138e...b15dc7e. Read the comment docs.

shoyer · 2019-01-30T04:46:08Z

Following up on
#25007 (comment), IMO Index.intersection(sort=None) is not needed.

Agreed!

jorisvandenbossche · 2019-01-30T07:03:03Z

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 !

doc/source/whatsnew/v0.24.1.rst

jorisvandenbossche · 2019-01-30T07:05:12Z

doc/source/whatsnew/v0.24.1.rst

+
+When ``sort=True`` is provided to :meth:`Index.intersection`, the values are always sorted. In 0.24.0,
+the values would not be sorted when ``self`` and ``other`` were identical. Pass ``sort=False`` to not
+sort the values.


Maybe we should make it clearer that this was new behaviour in 0.24.0, and that no behaviour changed compared to what you could do on 0.23 ?

jreback · 2019-01-30T12:46:33Z

doc/source/whatsnew/v0.24.1.rst

+^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+When ``sort=True`` is provided to :meth:`Index.intersection`, the values are always sorted. In 0.24.0,
+the values would not be sorted when ``self`` and ``other`` were identical. Pass ``sort=False`` to not


I am -1 on this change. We do NOT do this elsewhere, e.g. .reindex, so this is extra useless sorting. (basically cases 1 and 2 above). I am not sure of the utility of 3 at all. We cannot guarantee sorting, showing a warning is fine ; this has been this way since pandas inception. I don't see any utility in changing this.

jorisvandenbossche · 2019-01-30T13:10:56Z

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?

jreback · 2019-01-30T13:13:33Z

@jorisvandenbossche .reindex does not reindex if the other is identical (or empty). This is the same exact situation

jorisvandenbossche · 2019-01-30T13:16:15Z

.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.

jreback · 2019-01-30T13:18:19Z

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.

jreback · 2019-01-30T13:20:24Z

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.

TomAugspurger · 2019-01-31T13:58:07Z

Did the same change for symmdiff, so that sort=True means sort.

jorisvandenbossche · 2019-01-31T14:29:16Z

Did the same change for symmdiff, so that sort=True means sort.

The same also needs to be done for difference ?

TomAugspurger · 2019-01-31T14:31:02Z

Sorry, yes...

jreback · 2019-01-31T14:34:43Z

I have not wavered on this and am -1

I see no reason to not simply do the change in 0.25

jorisvandenbossche · 2019-01-31T14:45:08Z

I see no reason to not simply do the change in 0.25

Because then we would need to deprecate the sort=True keyword we now introduced in 0.24.0. By quickly changing it, our goal is to avoid this deprecation.

@jreback can you please be more specific on what you are -1? Which of the following things do you object:

the long-term goal to have sort=False the default (meaning we would eg deprecate the default in 0.25.0)
the long-term goal that sort=True should mean "always sorting", even when the passed data is equal or empty
changing the default value from True to None (to preserve True for actually always sorting)
adding new behaviour to correspond to sort=True

jorisvandenbossche · 2019-01-31T14:46:05Z

(would you be able to come to gitter? that might be easier to try to come to an agreement)

TomAugspurger · 2019-01-31T14:47:16Z

I have not wavered on this and am -1

No! From

I am ok with sort=None if the ultimate goal is a deprecation so we can reach sort=False as a default.

I thought you were OK with it 😢

TomAugspurger · 2019-01-31T14:48:27Z

I really, really, really, think we should be doing this soon.

FYI, MultiIndex.difference was different from Index.difference in two ways

It short-circuited when other was empty (no sorting).
It didn't do a try / except around sorting.

(haven't fixed this yet).

jreback · 2019-01-31T14:49:08Z

adding new behaviour to correspond to sort=True

this is the problem

jreback · 2019-01-31T15:14:46Z

For the record I am -1 on this change as is, but @jorisvandenbossche and @TomAugspurger are going ahead.

TomAugspurger · 2019-01-31T15:14:54Z

Since MultiIndex.difference didn't have the silent "don't sort if not possible behavior", I haven't chosen to implement it yet.

# 0.23.4
In [25]: a = pd.MultiIndex.from_product([[1, pd.Timestamp('2000'), 0], [1, 2]])

In [26]: a.difference([(0, 1)])
---------------------------------------------------------------------------
TypeError                                 Traceback (most recent call last)
<ipython-input-26-d5517c8a9130> in <module>
----> 1 a.difference([(0, 1)])

~/miniconda3/envs/pandas-0.24.0/lib/python3.7/site-packages/pandas/core/indexes/multi.py in difference(self, other, sort)
   2984         difference = this.values.take(label_diff)
   2985         if sort:
-> 2986             difference = sorted(difference)
   2987
   2988         if len(difference) == 0:

pandas/_libs/tslibs/timestamps.pyx in pandas._libs.tslibs.timestamps._Timestamp.__richcmp__()

TypeError: Cannot compare type 'Timestamp' with type 'int'

do people have thoughts on that?

TomAugspurger · 2019-01-31T15:15:23Z

For the record I am -1 on this change as is, but @jorisvandenbossche and @TomAugspurger are going ahead.

@jreback not if you're -1. Pandas operates by consensus.

jreback · 2019-01-31T15:20:35Z

@TomAugspurger if I am -1 then there is clearly not consensus, as my concerns as laid out have not been addressed. This is way to many and too fast changes for a minor release.

jorisvandenbossche · 2019-01-31T15:23:08Z

@jreback concrete proposal:

for 0.24.1, we do:
- change the default value of sort=True to sort=None (of course without any change in behaviour, and also without adding new behaviour)
- disallow sort=True (just following from only having sort=None|False as options), which should not be a problem since nobody should already rely on this option (only introduced in 0.24.0 and it is the default anyway)
for 0.25.0, we can then:
- further discuss what we want for sort=True (the same behaviour as sort=None (what you want), or the "always sorting" behaviour (what Tom, Stephan and I prefer))
- further discuss about deprecation towards sort=False as default

What do you think about this? It would allow us to go forward with releasing 0.24.1today without needing to resolve the full discussion today.

And if not OK, can you please be specific what in there does not address your concerns?

TomAugspurger · 2019-01-31T15:25:00Z

If you’re -1 then we just aren’t doing it.

…

________________________________ From: Jeff Reback <notifications@github.com> Sent: Thursday, January 31, 2019 09:20 To: pandas-dev/pandas Cc: Tom Augspurger; Mention Subject: Re: [pandas-dev/pandas] API: Change default for Index.union sort (#25007) @TomAugspurger<https://github.com/TomAugspurger> if I am -1 then there is clearly not consensus, as my concerns as laid out have not been addressed. This is way to many and too fast changes for a minor release. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub<#25007 (comment)>, or mute the thread<https://github.com/notifications/unsubscribe-auth/ABQHIj528BnanUEKFqm-S3gxu71NM5kcks5vIwnIgaJpZM4aYnPM>.

jreback · 2019-01-31T15:26:24Z

@jorisvandenbossche your proposal is fine.

TomAugspurger · 2019-01-31T16:11:00Z

@jorisvandenbossche do you have time to implement
#25007 (comment)?

jorisvandenbossche · 2019-01-31T16:17:39Z

@TomAugspurger yes, will do that

TomAugspurger · 2019-02-04T22:12:40Z

Opened #25151 for the rest.

[WIP]: API: Change default for Index.union sort

aac172c

Closes pandas-dev#24959

TomAugspurger mentioned this pull request Jan 29, 2019

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

Closed

shoyer reviewed Jan 29, 2019

View reviewed changes

pandas/core/indexes/base.py Outdated

.. versionadded:: 0.24.0

.. versionchanged:: 0.24.0

Copy link

Member

shoyer Jan 29, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be "0.24.1"

update test

d4bcc55

jorisvandenbossche reviewed Jan 29, 2019

View reviewed changes

jreback requested changes Jan 29, 2019

View reviewed changes

TomAugspurger added 2 commits January 29, 2019 20:38

fixups

45c827c

Merge remote-tracking branch 'upstream/master' into 24959-union-default

68b72a6

TomAugspurger added 2 commits January 29, 2019 20:44

multi

8716f97

doc typo

f7056d5

TomAugspurger added 2 commits January 29, 2019 21:03

intersection

e82cbb1

whatsnew

2a2de25

jorisvandenbossche added this to the 0.24.1 milestone Jan 30, 2019

jorisvandenbossche reviewed Jan 30, 2019

View reviewed changes

jreback requested changes Jan 30, 2019

View reviewed changes

jreback removed this from the 0.24.1 milestone Jan 30, 2019

TomAugspurger added 4 commits January 31, 2019 07:40

Merge remote-tracking branch 'upstream/master' into 24959-union-default

ce6d1db

symdiff

52a2f2f

whatsnew

bb848f1

doc

b15dc7e

TomAugspurger changed the title ~~[WIP]: API: Change default for Index.union sort~~ API: Change default for Index.union sort Jan 31, 2019

index multi

27b5b16

jreback approved these changes Jan 31, 2019

View reviewed changes

jorisvandenbossche mentioned this pull request Jan 31, 2019

API: change Index set ops sort=True -> sort=None #25063

Merged

TomAugspurger closed this Feb 4, 2019

TomAugspurger deleted the 24959-union-default branch February 4, 2019 22:12

wence- mentioned this pull request Nov 21, 2023

BUG: Discrepency between documentation and output for outer merge on index when left and right indices match and are unique #55992

Closed

3 tasks

API: Change default for Index.union sort #25007

API: Change default for Index.union sort #25007

Conversation

TomAugspurger commented Jan 29, 2019

shoyer Jan 29, 2019

Choose a reason for hiding this comment

jorisvandenbossche left a comment

Choose a reason for hiding this comment

TomAugspurger commented Jan 29, 2019

jorisvandenbossche commented Jan 29, 2019

TomAugspurger commented Jan 29, 2019

jreback Jan 29, 2019

Choose a reason for hiding this comment

jorisvandenbossche Jan 29, 2019

Choose a reason for hiding this comment

jreback Jan 29, 2019

Choose a reason for hiding this comment

TomAugspurger commented Jan 30, 2019

TomAugspurger commented Jan 30, 2019

codecov bot commented Jan 30, 2019

Codecov Report

codecov bot commented Jan 30, 2019 • edited Loading

Codecov Report

shoyer commented Jan 30, 2019

jorisvandenbossche commented Jan 30, 2019

jorisvandenbossche Jan 30, 2019

Choose a reason for hiding this comment

jreback Jan 30, 2019

Choose a reason for hiding this comment

jorisvandenbossche commented Jan 30, 2019

jreback commented Jan 30, 2019

jorisvandenbossche commented Jan 30, 2019

jreback commented Jan 30, 2019

jreback commented Jan 30, 2019

TomAugspurger commented Jan 31, 2019

jorisvandenbossche commented Jan 31, 2019

TomAugspurger commented Jan 31, 2019

jreback commented Jan 31, 2019

jorisvandenbossche commented Jan 31, 2019 • edited Loading

jorisvandenbossche commented Jan 31, 2019

TomAugspurger commented Jan 31, 2019

TomAugspurger commented Jan 31, 2019 • edited Loading

jreback commented Jan 31, 2019

jreback commented Jan 31, 2019

TomAugspurger commented Jan 31, 2019

TomAugspurger commented Jan 31, 2019

jreback commented Jan 31, 2019

jorisvandenbossche commented Jan 31, 2019 • edited Loading

TomAugspurger commented Jan 31, 2019 via email

jreback commented Jan 31, 2019

TomAugspurger commented Jan 31, 2019

jorisvandenbossche commented Jan 31, 2019

TomAugspurger commented Feb 4, 2019

codecov bot commented Jan 30, 2019 •

edited

Loading

jorisvandenbossche commented Jan 31, 2019 •

edited

Loading

TomAugspurger commented Jan 31, 2019 •

edited

Loading

jorisvandenbossche commented Jan 31, 2019 •

edited

Loading