ENH: Add sort parameter to set operations for some Indexes and adjust… #24521

reidy-p · 2018-12-31T20:09:54Z

… tests

Progress towards ENH: Add sort parameter to other set operations if possible #24471
tests added / passed
passes git diff upstream/master -u -- "*.py" | flake8 --diff
whatsnew entry

This PR makes some progress towards adding a sort parameter with a default of True to the set operations (union, intersection, difference and symmetric_difference) on Index classes. Adding this parameter into every type of Index would result in a very big PR so I have decided to break it up and try to add it in stages. I have tried to focus on Index, MultiIndex, PeriodIndex and Intervalndex in this PR but have made some very small changes to set operations on other indices where necessary.

Some issues to consider:

I'm not sure whether it will be possible to control the sorting behaviour of the results of all of the set operations for all of the Index types. For example, because of the way some of the set operations are implemented on some Index types the results may always be sorted even if sort=False is passed (e.g., a union operation on DatetimeIndexs may always return a sorted result in some cases even if sort=False). Perhaps this is not really a problem as long as we document it.
There are some other corner cases that always ignore the sort parameter at the moment. For example, the intersection of an unsorted Index with itself will return the original unsorted Index even if sort=True is passed because the sort parameter is simply ignored in this case. Similarly, the intersection of an unsorted Index with an empty Index will also return the original unsorted Index even if sort=True. There is similar behaviour for the other set operations.

pandas/_libs/lib.pyx

pandas/core/indexes/interval.py

reidy-p · 2018-12-31T20:20:14Z

pandas/core/indexes/datetimes.py

@@ -603,7 +603,7 @@ def _wrap_setop_result(self, other, result):
            raise ValueError('Passed item and index have different timezone')
        return self._shallow_copy(result, name=name, freq=None, tz=self.tz)

-    def intersection(self, other):
+    def intersection(self, other, sort=True):


The sort parameter is not fully implemented for intersection on DatetimeIndex yet. I just put this in for now to fix some failing tests

pandas/tests/indexes/multi/test_set_ops.py

codecov · 2018-12-31T20:32:38Z

Codecov Report

Merging #24521 into master will increase coverage by 0.01%.
The diff coverage is 23.25%.

@@            Coverage Diff             @@
##           master   #24521      +/-   ##
==========================================
+ Coverage   31.88%    31.9%   +0.01%     
==========================================
  Files         166      166              
  Lines       52434    52444      +10     
==========================================
+ Hits        16718    16730      +12     
+ Misses      35716    35714       -2

Flag	Coverage Δ
#multiple	`30.28% <16.27%> (-0.01%)`	⬇️
#single	`31.9% <23.25%> (+0.01%)`	⬆️

Impacted Files	Coverage Δ
pandas/core/indexes/interval.py	`35.03% <0%> (+0.12%)`	⬆️
pandas/core/indexes/api.py	`78.21% <0%> (ø)`	⬆️
pandas/core/indexes/base.py	`47.46% <20.68%> (+0.44%)`	⬆️
pandas/core/indexes/range.py	`48.97% <25%> (-0.29%)`	⬇️
pandas/core/indexes/datetimes.py	`37.33% <33.33%> (ø)`	⬆️
pandas/core/indexes/multi.py	`30.58% <40%> (-0.05%)`	⬇️

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 bf9d41c...4b31df2. Read the comment docs.

codecov · 2018-12-31T20:32:44Z

Codecov Report

Merging #24521 into master will decrease coverage by <.01%.
The diff coverage is 94.59%.

@@            Coverage Diff             @@
##           master   #24521      +/-   ##
==========================================
- Coverage   92.38%   92.38%   -0.01%     
==========================================
  Files         166      166              
  Lines       52382    52392      +10     
==========================================
+ Hits        48395    48404       +9     
- Misses       3987     3988       +1

Flag	Coverage Δ
#multiple	`90.81% <91.89%> (ø)`	⬆️
#single	`42.91% <45.94%> (ø)`	⬆️

Impacted Files	Coverage Δ
pandas/core/indexes/multi.py	`95.59% <100%> (ø)`	⬆️
pandas/core/indexes/datetimes.py	`96.27% <100%> (ø)`	⬆️
pandas/core/indexes/api.py	`99% <100%> (ø)`	⬆️
pandas/core/indexes/interval.py	`95.25% <100%> (-0.02%)`	⬇️
pandas/io/pytables.py	`92.31% <100%> (ø)`	⬆️
pandas/core/indexes/range.py	`97.36% <100%> (+0.01%)`	⬆️
pandas/core/indexes/base.py	`96.32% <90.9%> (+0.01%)`	⬆️
pandas/util/testing.py	`88.04% <0%> (-0.1%)`	⬇️

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 aa05e54...7b15248. Read the comment docs.

jreback

looks pretty good.

pandas/_libs/lib.pyx

jreback · 2018-12-31T23:40:28Z

pandas/core/indexes/base.py

+            try:
+                taken = taken.sort_values()
+            except TypeError as e:
+                warnings.warn("%s, sort order is undefined for "


does this actually get hit? I really hate all of these sort warnings. maybe can you factor them out in a helper function instead?

can you use .format here

pandas/core/indexes/base.py

pandas/tests/indexes/multi/test_set_ops.py

jreback · 2018-12-31T23:46:09Z

so we can followup up on the 2 cases you mentioned at the top of the PR. pls roll them into an issue (you can expand with check boxes if more clear).

pandas/core/indexes/base.py

reidy-p · 2019-01-12T15:38:10Z

pandas/core/indexes/base.py

        elif how == 'outer':
-            join_index = self.union(other, sort=sort)
+            join_index = self.union(other)


On Master union does generally sort by default so I have left the default sort=True here for now for compatibility reasons so the current behaviour of join(..., how='outer') does not change for now and all of the tests expecting the results of this type of join to be sorted don't break.

reidy-p · 2019-01-12T15:44:34Z

Updated the original issue (#24471) with some of the corner cases I mention above. I will try to address these in subsequent PRs once this PR is finished.

reidy-p · 2019-01-12T17:11:33Z

pandas/io/pytables.py

@@ -4473,7 +4473,7 @@ def _reindex_axis(obj, axis, labels, other=None):

    labels = ensure_index(labels.unique())
    if other is not None:
-        labels = ensure_index(other.unique()) & labels
+        labels = ensure_index(other.unique()).intersection(labels, sort=False)


Again, on Master intersection does not generally sort by default so I have added sort=False here for compatibility

reidy-p · 2019-01-12T18:38:52Z

pandas/tests/indexes/test_base.py

@@ -2359,12 +2359,14 @@ def test_intersection_base(self, sort):
        if PY3 and sort:
            # unorderable types
            warn_type = RuntimeWarning
+            expected = Index([0, 'a', 1])


At the moment Index.intersection does not use sorting.safe_sort which tries to sort arrays of mixed type (int and str) by ordering int before str in Python 3. I could try to change this either in this PR or in a separate one if this is getting too big already.

Decided to use sort_safe in Index.intersection in this PR in the next commit

jreback

small comments. ping on green.

jreback · 2019-01-13T21:57:28Z

pandas/_libs/lib.pyx

-        pass
+    if sort:
+        try:
+            uniques.sort()


might see if using safe_sort makes sense here (you would have to import here, otherwise this would e circular)

I tried using safe_sort here but it was causing some problems. The issue seems to be that uniques here is a list of tuples which are then used to construct a MultiIndex in MultiIndex.union. However, when we use safe_sort it turns uniques into an np.array and doesn't sort it correctly so we get the wrong results. I can have a closer look at trying to resolve this if you want but it might involve changing safe_sort a bit.

ok, let's followup up later then (new PR).

jreback · 2019-01-13T21:58:56Z

pandas/core/indexes/base.py

+        if sort:
+            try:
+                taken = sorting.safe_sort(taken.values)
+                if self.name != other.name:


use
_get_reconciled_name_object

I tried using get_reconciled_name_object but from the docstring it says that it's used when the "result of the set operation will be self, return self, unless the name changes, in which case make a shallow copy of self". But the result of the set operation is not self in this case, I think.

jreback · 2019-01-13T22:01:00Z

pandas/core/indexes/base.py

+            try:
+                taken = taken.sort_values()
+            except TypeError as e:
+                warnings.warn("%s, sort order is undefined for "


pandas/core/indexes/base.py

jreback · 2019-01-13T22:02:21Z

pandas/core/indexes/base.py

        elif how == 'outer':
-            join_index = self.union(other, sort=sort)
+            join_index = self.union(other)


jreback · 2019-01-15T12:10:30Z

pandas/_libs/lib.pyx

-        pass
+    if sort:
+        try:
+            uniques.sort()


jreback · 2019-01-15T12:10:48Z

pandas/core/indexes/base.py

+                    result = sorting.safe_sort(result)
+                except TypeError as e:
+                    warnings.warn("%s, sort order is undefined for "
+                                  "incomparable objects" % e, RuntimeWarning,


can you use .format() here

jreback · 2019-01-15T12:11:01Z

pandas/core/indexes/base.py

+            try:
+                taken = taken.sort_values()
+            except TypeError as e:
+                warnings.warn("%s, sort order is undefined for "


can you use .format here

… tests

…this

pep8speaks · 2019-01-19T18:20:35Z

Hello @reidy-p! Thanks for updating the PR.

Cheers ! There are no PEP8 issues in this Pull Request. 🍻

Comment last updated on January 19, 2019 at 18:22 Hours UTC

jreback

couple of comments, but let's do in a followup if you can (otherwise pls make an issue).

jreback · 2019-01-19T20:58:09Z

pandas/_libs/lib.pyx

-        pass
+    if sort:
+        try:
+            uniques.sort()


ok, let's followup up later then (new PR).

jreback · 2019-01-19T21:04:13Z

pandas/core/indexes/base.py

@@ -2385,8 +2393,18 @@ def intersection(self, other):
            indexer = indexer[indexer != -1]

        taken = other.take(indexer)
+
+        if sort:


ok, I still think we can use _get_reconcile_name_object here (maybe need to wrap taken in Index), but can do in a followup.

jreback · 2019-01-19T21:05:01Z

thanks @reidy-p

reidy-p · 2019-01-19T21:07:07Z

@jreback thanks. I hope to do a few follow-up PRs on this.

shoyer · 2019-01-27T00:59:33Z

Was it intended to change the default behavior of Index.intersection here? Previously intersection never sorted, but now it sorts by default.

jreback · 2019-01-27T01:02:36Z

all of the set ops always sorted; the parameter now allows u to turn it off (default should be the same as before)

shoyer · 2019-01-27T01:10:28Z

This is definitely changed behavior from 0.23.x for intersection (it turned up in a failure in xarray's test suite), but I'll raise another issue about that.

shoyer · 2019-01-27T01:19:41Z

See #24959 for discussion about the behavior change.

pandas-dev#24521)

reidy-p commented Dec 31, 2018

View reviewed changes

pandas/_libs/lib.pyx Show resolved Hide resolved

reidy-p commented Dec 31, 2018

View reviewed changes

pandas/core/indexes/interval.py Show resolved Hide resolved

reidy-p commented Dec 31, 2018

View reviewed changes

pandas/tests/indexes/multi/test_set_ops.py Show resolved Hide resolved

jreback requested changes Dec 31, 2018

View reviewed changes

jreback added the Reshaping Concat, Merge/Join, Stack/Unstack, Explode label Dec 31, 2018

reidy-p mentioned this pull request Jan 1, 2019

CLN: Refactor some sorting code in Index set operations #24533

Merged

3 tasks

reidy-p force-pushed the sort_parameter_set_operations branch from f3b517a to f393803 Compare January 12, 2019 15:33

reidy-p commented Jan 12, 2019

View reviewed changes

pandas/core/indexes/base.py Show resolved Hide resolved

reidy-p commented Jan 12, 2019

View reviewed changes

jreback requested changes Jan 13, 2019

View reviewed changes

jreback added this to the 0.24.0 milestone Jan 13, 2019

jreback added the API Design label Jan 13, 2019

jreback requested changes Jan 15, 2019

View reviewed changes

reidy-p added 6 commits January 19, 2019 11:24

ENH: Add sort parameter to set operations for some Indexes and adjust…

6afbefe

… tests

Pass sort parameter into union in join function

1071f6b

Add docstring and change sorting in join function for compatibility

fcdd676

Keep default sort=False for intersection in pytables

97b0895

Index.intersection does not use safe_sort so adjust tests to reflect …

2edebf1

…this

Use safe_sort in Index.intersection

67a4e58

reidy-p force-pushed the sort_parameter_set_operations branch from d9e68be to 9549661 Compare January 19, 2019 18:20

Adjust warnings and add TODOs

7b15248

reidy-p force-pushed the sort_parameter_set_operations branch from 9549661 to 7b15248 Compare January 19, 2019 18:22

jreback approved these changes Jan 19, 2019

View reviewed changes

jreback merged commit bd3c001 into pandas-dev:master Jan 19, 2019

reidy-p deleted the sort_parameter_set_operations branch January 26, 2019 18:11

shoyer mentioned this pull request Jan 27, 2019

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

Closed

Pingviinituutti pushed a commit to Pingviinituutti/pandas that referenced this pull request Feb 28, 2019

ENH: Add sort parameter to set operations for some Indexes and adjust… (

b54f091

pandas-dev#24521)

Pingviinituutti pushed a commit to Pingviinituutti/pandas that referenced this pull request Feb 28, 2019

ENH: Add sort parameter to set operations for some Indexes and adjust… (

35da4eb

pandas-dev#24521)

ENH: Add sort parameter to set operations for some Indexes and adjust… #24521

ENH: Add sort parameter to set operations for some Indexes and adjust… #24521

Conversation

reidy-p commented Dec 31, 2018 • edited Loading

Choose a reason for hiding this comment

codecov bot commented Dec 31, 2018

Codecov Report

codecov bot commented Dec 31, 2018 • edited Loading

Codecov Report

jreback left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jreback commented Dec 31, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

reidy-p commented Jan 12, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jreback left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pep8speaks commented Jan 19, 2019 • edited Loading

Comment last updated on January 19, 2019 at 18:22 Hours UTC

jreback left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jreback commented Jan 19, 2019

reidy-p commented Jan 19, 2019

shoyer commented Jan 27, 2019

jreback commented Jan 27, 2019

shoyer commented Jan 27, 2019

shoyer commented Jan 27, 2019

reidy-p commented Dec 31, 2018 •

edited

Loading

codecov bot commented Dec 31, 2018 •

edited

Loading

pep8speaks commented Jan 19, 2019 •

edited

Loading