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

- [x] closes #17376
- [x] tests added / passed
- [x] passes `git diff master -u -- "*.py" | flake8 --diff`
- [x] 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.

Labels:
name: Bug
name: Reshaping
description: Concat, Merge/Join, Stack/Unstack, Explode

Comments:
# [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/17378?src=pr&el=h1) Report
> Merging [#17378](https://codecov.io/gh/pandas-dev/pandas/pull/17378?src=pr&el=desc) into [master](https://codecov.io/gh/pandas-dev/pandas/commit/0d676a3ccf1d7aa986416a7488b941496f936d98?src=pr&el=desc) will **decrease** coverage by `0.01%`.
> The diff coverage is `100%`.

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

```diff
@@            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%)` | :arrow_down: |
| #single | `40.25% <60%> (-0.07%)` | :arrow_down: |

| [Impacted Files](https://codecov.io/gh/pandas-dev/pandas/pull/17378?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pandas/core/indexes/base.py](https://codecov.io/gh/pandas-dev/pandas/pull/17378?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvaW5kZXhlcy9iYXNlLnB5) | `95.86% <100%> (-0.02%)` | :arrow_down: |
| [pandas/io/gbq.py](https://codecov.io/gh/pandas-dev/pandas/pull/17378?src=pr&el=tree#diff-cGFuZGFzL2lvL2dicS5weQ==) | `25% <0%> (-58.34%)` | :arrow_down: |
| [pandas/core/frame.py](https://codecov.io/gh/pandas-dev/pandas/pull/17378?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvZnJhbWUucHk=) | `97.72% <0%> (-0.1%)` | :arrow_down: |

------

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

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

```diff
@@            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%)` | :arrow_down: |
| #single | `41.86% <60%> (ø)` | :arrow_up: |

| [Impacted Files](https://codecov.io/gh/pandas-dev/pandas/pull/17378?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pandas/core/indexes/base.py](https://codecov.io/gh/pandas-dev/pandas/pull/17378/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvaW5kZXhlcy9iYXNlLnB5) | `96.65% <100%> (-0.02%)` | :arrow_down: |
| [pandas/core/reshape/tile.py](https://codecov.io/gh/pandas-dev/pandas/pull/17378/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvcmVzaGFwZS90aWxlLnB5) | `92.94% <0%> (-0.43%)` | :arrow_down: |
| [pandas/core/window.py](https://codecov.io/gh/pandas-dev/pandas/pull/17378/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvd2luZG93LnB5) | `96.31% <0%> (ø)` | :arrow_up: |

------

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

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.
@toobaz : Nope, not a clue :cry: .  There's no way your changes AFAICT could have caused internal errors on the build.  I restarted AppVeyor to double check.
> I restarted AppVeyor to double check.

Failed again... shall I try closing this and opening a new PR?
> Failed again... shall I try closing this and opening a new PR?

NO. that just creates more work. let me look.
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).

it *looks* like its either segfaulting or in a recursive loop somewhere.
> setup a vm on 3.6/windows is easiest / best.

Do you mean Python 3.6?! (tests fail only on 2.7)
@toobaz  : I think @jreback probably meant 2.7 :smile: 
@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 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``...
> 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.
can you rebase. I think we do want to add this kw.
Unfortunately the problem on Windows + Python 2.7 persists. I haven't found the time to set up a virtual machine to debug.
@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.
closing as stale. would like to generaly add the ``sort=`` kwarg though.
@jreback : Let me take a crack at reviving this.
@gfyoung great! If you want a rebase, feel free to ask.
There's a lint error on Travis, but I want to check the other builds before pushing the fix.
@toobaz : That AppVeyor error looks puzzling.  Could be something with the build itself, as I can't see that "internal error" occurring locally at least.
I don't think it conflicts much with this PR, but FYI I have a branch started adding `sort` to `union`. https://github.com/pandas-dev/pandas/compare/master...TomAugspurger:index-sort

I think it works for every index except `RangeIndex`, since some `RangeIndex.union(RangeIndex)` ops that return a RangeIndex with `sort=True` won't return a `RangeIndex` with `sort=False`.
@TomAugspurger : Good to know.  Thanks!
@toobaz : also not sure why you're running up against some random `clip` errors on Travis.  I can't reproduce those locally.
I'm guessing it's a change to NumPy dev:
https://github.com/pandas-dev/pandas/issues/19976 (haven't tried locally
though)

On Fri, Mar 2, 2018 at 3:44 PM, gfyoung <notifications@github.com> wrote:

> @toobaz <https://github.com/toobaz> : also not sure why you're running up
> against some random clip errors on Travis. I can't reproduce those
> locally.
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub
> <https://github.com/pandas-dev/pandas/pull/17378#issuecomment-370061329>,
> or mute the thread
> <https://github.com/notifications/unsubscribe-auth/ABQHIjAXOWBI_zEkcoeT0sOE4y4oscp3ks5tab0zgaJpZM4PG7KR>
> .
>

> I'm guessing it's a change to NumPy dev:

@TomAugspurger : That seems reasonable.  I don't see any recent PR's for `clip`.
@toobaz : Despite the massive typo in my commit message, the latest commit is a dummy one to determine if that last AppVeyor build is just busted, or there is potentially an issue in the code.  I will undo that commit of course to restore the changes back in the PR.
> @toobaz : Despite the massive typo in my commit message, the latest commit is a dummy one to determine if that last AppVeyor build is just busted, or there is potentially an issue in the code

Fingers crossed... but I had triggered that behavior consistently (on multiple rebases).
@toobaz : Not looking good here...the build passed just fine when I undid all of your changes.
So adding more verbosity to **pytest** appears to have given a hint as to the problematic tests:

https://ci.appveyor.com/project/pandas-dev/pandas/build/1.0.11213/job/36lok03ygfrk2g86#L5503