BUG: Try to sort result of Index.union rather than guessing sortability #17378

toobaz · 2017-08-30T06:30:30Z

closes Union of indexes of tuples is not sorted #17376
tests added / passed
passes git diff master -u -- "*.py" | flake8 --diff
whatsnew entry

The lines of code I removed seemed written with the intention of guaranteeing the same result under Python 2 and 3, but the docs say "Form the union of two Index objects and sorts if possible.": so if it is possible in Python 2 we should do it, regardless of whether it is in Python 3. After all, this is how .sort_values() works. And more practically, trying to guess whether the result of an union can be sorted is hard.

codecov · 2017-08-30T07:52:01Z

Codecov Report

Merging #17378 into master will decrease coverage by 0.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master   #17378      +/-   ##
==========================================
- Coverage   91.01%   90.99%   -0.02%     
==========================================
  Files         163      163              
  Lines       49567    49559       -8     
==========================================
- Hits        45113    45096      -17     
- Misses       4454     4463       +9

Flag	Coverage Δ
#multiple	`88.77% <100%> (-0.01%)`	⬇️
#single	`40.25% <60%> (-0.07%)`	⬇️

Impacted Files	Coverage Δ
pandas/core/indexes/base.py	`95.86% <100%> (-0.02%)`	⬇️
pandas/io/gbq.py	`25% <0%> (-58.34%)`	⬇️
pandas/core/frame.py	`97.72% <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 0d676a3...35c2ec7. Read the comment docs.

codecov · 2017-08-30T07:52:13Z

Codecov Report

Merging #17378 into master will decrease coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master   #17378      +/-   ##
==========================================
- Coverage   91.72%   91.72%   -0.01%     
==========================================
  Files         150      150              
  Lines       49122    49104      -18     
==========================================
- Hits        45057    45039      -18     
  Misses       4065     4065

Flag	Coverage Δ
#multiple	`90.1% <100%> (-0.01%)`	⬇️
#single	`41.86% <60%> (ø)`	⬆️

Impacted Files	Coverage Δ
pandas/core/indexes/base.py	`96.65% <100%> (-0.02%)`	⬇️
pandas/core/reshape/tile.py	`92.94% <0%> (-0.43%)`	⬇️
pandas/core/window.py	`96.31% <0%> (ø)`	⬆️

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 7c14e4f...899c040. Read the comment docs.

jreback · 2017-08-30T10:11:26Z

pandas/core/indexes/base.py

-                    warnings.warn("%s, sort order is undefined for "
-                                  "incomparable objects" % e, RuntimeWarning,
-                                  stacklevel=3)
+            try:


you probably can simply use core.sorting.safe_sort here instead and just remove the warning entirely (as it fails in many less cases)

Done, but I left the warning as it's still raised if not even safe_sort can sort them.

(I assume the failure is a problem with AppVeyor)

toobaz · 2017-09-06T17:05:28Z

Any idea about what's happening with AppVeyor? It fails reliably on this PR with 2.7, but the trace doesn't point at any specific test.

gfyoung · 2017-09-06T17:23:44Z

@toobaz : Nope, not a clue 😢 . There's no way your changes AFAICT could have caused internal errors on the build. I restarted AppVeyor to double check.

toobaz · 2017-09-06T22:11:20Z

I restarted AppVeyor to double check.

Failed again... shall I try closing this and opening a new PR?

jreback · 2017-09-07T00:33:10Z

Failed again... shall I try closing this and opening a new PR?

NO. that just creates more work. let me look.

jreback · 2017-09-07T00:36:44Z

ok this is a legitimate catch. pytest-dist is segfaulting, hiding the traceback. So a couple of options to debug.

setup a vm on 3.6/windows is easiest / best.

you can temporarily change the options in the test runner (in ci/scripts_multiple.py) to something like:
pytest -v -r xX -m "not single" --junitxml=/tmp/multiple.xml $TEST_ARGS pandas

which disables the multi-processing and shows each test (so you can see where its failing).

jreback · 2017-09-07T00:37:33Z

it looks like its either segfaulting or in a recursive loop somewhere.

toobaz · 2017-09-07T05:17:20Z

setup a vm on 3.6/windows is easiest / best.

Do you mean Python 3.6?! (tests fail only on 2.7)

gfyoung · 2017-09-07T05:18:26Z

@toobaz : I think @jreback probably meant 2.7 😄

jreback · 2017-10-14T19:24:58Z

@toobaz this is related to #17839, could simply recast this to add the sort= kwarg (I think for back compat we have to use =False, but maybe be able to change that, not sure).

toobaz · 2017-10-15T07:12:50Z

@toobaz this is related to #17839, could simply recast this to add the sort= kwarg (I think for back compat we have to use =False, but maybe be able to change that, not sure).

Certainly related, but I'm confused: are you suggesting the two methods should share more code? Or just that Index.union too should have a sort= argument? In this case I think that for backward compatibility it should default to True...

jreback · 2017-10-31T01:22:55Z

Certainly related, but I'm confused: are you suggesting the two methods should share more code? Or just that Index.union too should have a sort= argument? In this case I think that for backward compatibility it should default to True...

yes I would add a sort=True kwarg.

jreback · 2017-11-25T16:20:48Z

can you rebase. I think we do want to add this kw.

toobaz · 2017-11-26T00:33:01Z

Unfortunately the problem on Windows + Python 2.7 persists. I haven't found the time to set up a virtual machine to debug.

jreback · 2018-01-21T18:17:28Z

@toobaz if you can rebase would be great. I would also be ok with simply adding the keyword w/o changing much code as a first step.

gfyoung · 2018-03-03T22:19:31Z

Hmmm...so this issue might be known with pytest (or its plugins) already:

pytest-dev/pytest#2223

Still not sure how @toobaz is triggering that though.

Unless anyone has any other insights into this, I feel inclined to file an issue against one of the pytest repositories and see what they have to say.

toobaz · 2018-03-04T09:35:36Z

Maybe we could ask whether there is a way to use this catchsegv within a pytest run?

gfyoung · 2018-03-06T23:04:49Z

Maybe we could ask whether there is a way to use this catchsegv within a pytest run?

@toobaz : Well, this is a Windows machine, not UNIX 😢

I notice that we are using pytest-forked, which AFAICT, isn't well-supported on Windows:

https://github.com/pytest-dev/pytest-forked

I was about to file an issue against pytest-xdist, but I wonder if this might be the problem child.

toobaz · 2018-03-06T23:20:48Z

@toobaz : Well, this is a Windows machine, not UNIX cry

right 🙄

I was about to file an issue against pytest-xdist, but I wonder if this might be the problem child.

What makes you think this is a bug in pytest, not a segfault somewhere in pandas? (or do you think it's both?)

gfyoung · 2018-03-07T02:27:51Z

What makes you think this is a bug in pytest, not a segfault somewhere in pandas? (or do you think it's both?)

I'm highly skeptical that your changes are causing a segfault. That would be pretty impressive actually given that you're only touching high-level Python 😄

In addition, I've tested your changes on multiple OS's (Windows including). No segfault anywhere.

gfyoung · 2018-03-07T05:05:18Z

So removing xdist + rebasing somehow resulted in test failures on the first machine?

Going to leave this PR alone for now until master is patched,

toobaz · 2018-03-07T08:27:11Z

I'm highly skeptical that your changes are causing a segfault. That would be pretty impressive actually given that you're only touching high-level Python

Sure, that would require that my changes are triggering a bug somewhere in lower-level code... but I consider this more probable than a bug in pydist which affects precisely that test, in precisely this PR.

Then again, I'm definitely not an expert.

gfyoung · 2018-03-07T09:05:40Z

I suppose, but if it's a bug in your code, I haven't been able to reproduce it locally.

toobaz · 2018-03-07T09:21:46Z

I suppose, but if it's a bug in your code, I haven't been able to reproduce it locally.

Aha! I had missed that you were testing on Windows. Good to know.

closes pandas-dev#17376

This will probably slow down performance, but I can't see where the bug is in the changes.

jreback · 2018-03-10T01:14:23Z

appveyor.yml

@@ -73,7 +73,7 @@ install:
  - cmd: conda info -a

  # create our env
-  - cmd: conda create -n pandas python=%PYTHON_VERSION% cython pytest>=3.1.0 pytest-xdist
+  - cmd: conda create -n pandas python=%PYTHON_VERSION% cython pytest>=3.1.0


so does xdist cause the issues with this?

It's really hard to tell at the moment. I've just been experimenting. I'm just at a loss right now as to how that one build on AppVeyor is crashing is so badly with changes like these.

gfyoung · 2018-03-10T18:18:28Z

@toobaz @jreback : So good news (I guess?), I was able to replicate the AppVeyor failure locally.

gfyoung · 2018-03-10T21:36:00Z

@toobaz : It appears that you loosened the sorting restrictions too much, at least for Windows:

The test that is breaking AppVeyor is TestAlignment.test_basic_frame_alignment
The condition where len(indexer) > 0 is the problem. For some reason, I need to put back the comparison of lvals[0] and other_diff[0] to remove the break OR only trying sorting when in the else branch (i.e. when len(indexer) = 0).

toobaz · 2018-03-11T18:45:41Z

@gfyoung thanks for your analysis!

Does the problem arise with any specific combination of index types? (from args = product(...)

gfyoung · 2018-03-11T20:48:03Z

Does the problem arise with any specific combination of index types?

I can't tell unfortunately. It seems to have crashed with multiple types.

jreback · 2018-04-14T13:38:44Z

@toobaz whats' the status on this?

jreback · 2018-10-11T01:49:29Z

closing as stale, if you want to continue working, pls ping and we can re-open. you will need to merge master.

toobaz force-pushed the index_union_sort branch 2 times, most recently from 1ca8f9c to 35c2ec7 Compare August 30, 2017 07:51

jreback requested changes Aug 30, 2017

View reviewed changes

jreback added 2/3 Compat Bug Reshaping Concat, Merge/Join, Stack/Unstack, Explode labels Aug 30, 2017

toobaz force-pushed the index_union_sort branch 2 times, most recently from 0a0e0f2 to 0a725fc Compare August 30, 2017 14:07

toobaz force-pushed the index_union_sort branch 3 times, most recently from bc4b8ba to e533022 Compare September 6, 2017 15:23

jreback mentioned this pull request Oct 14, 2017

ENH: sort=bool keyword argument for index.difference #17839

Closed

toobaz force-pushed the index_union_sort branch from e533022 to 83398d9 Compare October 29, 2017 10:32

toobaz force-pushed the index_union_sort branch from 83398d9 to decaa47 Compare November 25, 2017 17:15

gfyoung force-pushed the index_union_sort branch 3 times, most recently from 156fd86 to f4571a2 Compare March 7, 2018 04:00

gfyoung force-pushed the index_union_sort branch from f4571a2 to 953ccc6 Compare March 9, 2018 02:14

toobaz and others added 3 commits March 9, 2018 20:01

BUG: Try to sort result of Index.union rather than guessing sortability

9fceb51

closes pandas-dev#17376

CLN: Fix flake8 errors

7e70fd7

[DO NOT MERGE] Is it pytest-xdist?

899c040

This will probably slow down performance, but I can't see where the bug is in the changes.

gfyoung force-pushed the index_union_sort branch from 953ccc6 to 899c040 Compare March 10, 2018 01:03

jreback reviewed Mar 10, 2018

View reviewed changes

toobaz added a commit to toobaz/pandas that referenced this pull request Apr 24, 2018

BUG: workaround failure of pandas-dev#17378 on Windows

8d2e40d

toobaz mentioned this pull request Apr 24, 2018

Index union sort hack #20809

Closed

4 tasks

toobaz added a commit to toobaz/pandas that referenced this pull request Apr 24, 2018

BUG: workaround failure of pandas-dev#17378 on Windows

c07e91e

jreback mentioned this pull request Sep 25, 2018

BUG: Maintain column order with groupby.nth #22811

Merged

4 tasks

jreback closed this Oct 11, 2018

reidy-p mentioned this pull request Dec 31, 2018

ENH: Add sort parameter to other set operations if possible #24471

Closed

6 tasks

BUG: Try to sort result of Index.union rather than guessing sortability #17378

BUG: Try to sort result of Index.union rather than guessing sortability #17378

Conversation

toobaz commented Aug 30, 2017

codecov bot commented Aug 30, 2017

Codecov Report

codecov bot commented Aug 30, 2017 • edited Loading

Codecov Report

jreback Aug 30, 2017

Choose a reason for hiding this comment

toobaz Aug 30, 2017

Choose a reason for hiding this comment

toobaz commented Sep 6, 2017

gfyoung commented Sep 6, 2017

toobaz commented Sep 6, 2017

jreback commented Sep 7, 2017

jreback commented Sep 7, 2017

jreback commented Sep 7, 2017

toobaz commented Sep 7, 2017

gfyoung commented Sep 7, 2017

jreback commented Oct 14, 2017

toobaz commented Oct 15, 2017

jreback commented Oct 31, 2017

jreback commented Nov 25, 2017

toobaz commented Nov 26, 2017

jreback commented Jan 21, 2018

gfyoung commented Mar 3, 2018 • edited Loading

toobaz commented Mar 4, 2018

gfyoung commented Mar 6, 2018

toobaz commented Mar 6, 2018

gfyoung commented Mar 7, 2018 • edited Loading

gfyoung commented Mar 7, 2018

toobaz commented Mar 7, 2018

gfyoung commented Mar 7, 2018

toobaz commented Mar 7, 2018

jreback Mar 10, 2018

Choose a reason for hiding this comment

gfyoung Mar 10, 2018 • edited Loading

Choose a reason for hiding this comment

gfyoung commented Mar 10, 2018

gfyoung commented Mar 10, 2018 • edited Loading

toobaz commented Mar 11, 2018

gfyoung commented Mar 11, 2018

jreback commented Apr 14, 2018

jreback commented Oct 11, 2018

codecov bot commented Aug 30, 2017 •

edited

Loading

gfyoung commented Mar 3, 2018 •

edited

Loading

gfyoung commented Mar 7, 2018 •

edited

Loading

gfyoung Mar 10, 2018 •

edited

Loading

gfyoung commented Mar 10, 2018 •

edited

Loading