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

[toobaz]

• 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]

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]

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]

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

[toobaz]

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]

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]

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

I restarted AppVeyor to double check.

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

[jreback]

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

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

[jreback]

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]

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

[toobaz]

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

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

[gfyoung]

@toobaz : I think @jreback probably meant 2.7 😄

[jreback]

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

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

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]

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

[toobaz]

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

[jreback]

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

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]

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

[gfyoung]

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]

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

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]

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]

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]

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

[toobaz]

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.

[jreback]

so does xdist cause the issues with this?

[gfyoung]

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]

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

[gfyoung]

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

@gfyoung thanks for your analysis!

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

[gfyoung]

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]

@toobaz whats' the status on this?

[jreback]

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