ENH - Index set operation modifications to address issue #23525
State: closed

This is a first pass at addressing the associated issue. Needs plenty of discussion, feedback and testing.

- [ ] closes #23525
- [ ] tests added / passed
- [ ] passes `git diff upstream/master -u -- "*.py" | flake8 --diff`
- [ ] whatsnew entry


Labels:
name: Indexing
description: Related to indexing on series/frames, not to indexes themselves
name: Dtype Conversions
description: Unexpected or buggy dtype conversions
name: API Design
name: Period
description: Period data type

Comments:
Hello @ArtinSarraf! Thanks for updating this PR. We checked the lines you've touched for [PEP 8](https://www.python.org/dev/peps/pep-0008) issues, and found:









































There are currently no PEP 8 issues detected in this Pull Request. Cheers! :beers: 

##### Comment last updated at 2019-05-21 00:53:39 UTC
i would write tests before writing any code
@jreback / @TomAugspurger / @gfyoung  - Added tests for combinations of mismatched types. As well as for compatible inconsistent pairs (i.e. Range/Int64Index). Existing tests have been updated to account for the change in behavior and all `pytest pandas/tests` pass.

Any feedback on whether the changes are on the right track?
when you push always rebase again master.
@jreback - regarding moving setop tests into the base level:
Right now all the setop tests are specific to an Index type and are segregated into their own respective index subdirectory. Combining them all together would create a 1000+ line test module with mixed test styles which might be harder to parse through than the separate files. Should we leave as is, or would one module with clear section headers be sufficient?
@jschendel - Added your proposed solution for the difference operation. I left out "intersection" because it seems that this behavior was specific to DatetimeIndexes and no others.
Regarding the comment about addressing the mixed interval types, it looks like that was already covered at the bottom of that test method.
@jreback - any thoughts on the review responses. Especially these 2?
[Overriden public union methods](https://github.com/pandas-dev/pandas/pull/23538#discussion_r233669067)
[New concat resulting index dtype](https://github.com/pandas-dev/pandas/pull/23538#discussion_r232514941)
@jreback - looks like the only failures in the Travis CI were 2 tests from build #48415.4 in `pandas/tests/plotting/test_datetimelike.py`. These tests pass when I run locally, thinking it might be a transient error. Is there any way to rerun the build without submitting a new commit?

```
pandas/tests/plotting/test_datetimelike.py:89: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
pandas/tests/plotting/test_datetimelike.py:1561: in _check_plot_works
    pickle.dump(fig, path)
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
m = <bound method AffineBase.transform of <matplotlib.transforms.BlendedAffine2D object at 0x7fe3cc3dd780>>
    def _pickle_method(m):
        """Handle pickling issues with class instance."""
>       if m.im_self is None:
E       AttributeError: 'function' object has no attribute 'im_self'
```
# [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/23538?src=pr&el=h1) Report
> Merging [#23538](https://codecov.io/gh/pandas-dev/pandas/pull/23538?src=pr&el=desc) into [master](https://codecov.io/gh/pandas-dev/pandas/commit/f5cc0789ae5f5a9e5b251e94784f537f30e0b490?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/23538/graphs/tree.svg?width=650&token=eZ4WkYLtcO&height=150&src=pr)](https://codecov.io/gh/pandas-dev/pandas/pull/23538?src=pr&el=tree)

```diff
@@            Coverage Diff             @@
##           master   #23538      +/-   ##
==========================================
- Coverage   91.75%   91.73%   -0.02%     
==========================================
  Files         174      174              
  Lines       50765    50770       +5     
==========================================
- Hits        46578    46576       -2     
- Misses       4187     4194       +7
```

| Flag | Coverage Δ | |
|---|---|---|
| #multiple | `90.25% <100%> (-0.01%)` | :arrow_down: |
| #single | `41.72% <48.71%> (-0.09%)` | :arrow_down: |

| [Impacted Files](https://codecov.io/gh/pandas-dev/pandas/pull/23538?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pandas/core/indexes/timedeltas.py](https://codecov.io/gh/pandas-dev/pandas/pull/23538/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvaW5kZXhlcy90aW1lZGVsdGFzLnB5) | `90.96% <100%> (-0.63%)` | :arrow_down: |
| [pandas/core/indexes/period.py](https://codecov.io/gh/pandas-dev/pandas/pull/23538/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvaW5kZXhlcy9wZXJpb2QucHk=) | `92.58% <100%> (+0.47%)` | :arrow_up: |
| [pandas/core/indexes/interval.py](https://codecov.io/gh/pandas-dev/pandas/pull/23538/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvaW5kZXhlcy9pbnRlcnZhbC5weQ==) | `96.1% <100%> (+0.87%)` | :arrow_up: |
| [pandas/core/indexes/numeric.py](https://codecov.io/gh/pandas-dev/pandas/pull/23538/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvaW5kZXhlcy9udW1lcmljLnB5) | `97.34% <100%> (+0.03%)` | :arrow_up: |
| [pandas/core/indexes/datetimes.py](https://codecov.io/gh/pandas-dev/pandas/pull/23538/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvaW5kZXhlcy9kYXRldGltZXMucHk=) | `96.36% <100%> (-0.5%)` | :arrow_down: |
| [pandas/core/indexes/base.py](https://codecov.io/gh/pandas-dev/pandas/pull/23538/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvaW5kZXhlcy9iYXNlLnB5) | `96.73% <100%> (ø)` | :arrow_up: |
| [pandas/core/indexes/range.py](https://codecov.io/gh/pandas-dev/pandas/pull/23538/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvaW5kZXhlcy9yYW5nZS5weQ==) | `97.97% <100%> (-0.01%)` | :arrow_down: |
| [pandas/io/gbq.py](https://codecov.io/gh/pandas-dev/pandas/pull/23538/diff?src=pr&el=tree#diff-cGFuZGFzL2lvL2dicS5weQ==) | `78.94% <0%> (-10.53%)` | :arrow_down: |
| [pandas/core/dtypes/common.py](https://codecov.io/gh/pandas-dev/pandas/pull/23538/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvZHR5cGVzL2NvbW1vbi5weQ==) | `95.85% <0%> (-1.6%)` | :arrow_down: |
| ... and [4 more](https://codecov.io/gh/pandas-dev/pandas/pull/23538/diff?src=pr&el=tree-more) | |

------

[Continue to review full report at Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/23538?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/23538?src=pr&el=footer). Last update [f5cc078...b840f49](https://codecov.io/gh/pandas-dev/pandas/pull/23538?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/23538?src=pr&el=h1) Report
> Merging [#23538](https://codecov.io/gh/pandas-dev/pandas/pull/23538?src=pr&el=desc) into [master](https://codecov.io/gh/pandas-dev/pandas/commit/5b6b346b9db57b610df2965f60753473ef16e16f?src=pr&el=desc) will **increase** coverage by `<.01%`.
> The diff coverage is `60%`.

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

```diff
@@            Coverage Diff             @@
##           master   #23538      +/-   ##
==========================================
+ Coverage   42.46%   42.46%   +<.01%     
==========================================
  Files         161      161              
  Lines       51557    51590      +33     
==========================================
+ Hits        21892    21910      +18     
- Misses      29665    29680      +15
```

| Flag | Coverage Δ | |
|---|---|---|
| #single | `42.46% <60%> (ø)` | :arrow_up: |

| [Impacted Files](https://codecov.io/gh/pandas-dev/pandas/pull/23538?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pandas/core/indexes/interval.py](https://codecov.io/gh/pandas-dev/pandas/pull/23538/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvaW5kZXhlcy9pbnRlcnZhbC5weQ==) | `34.54% <0%> (-0.37%)` | :arrow_down: |
| [pandas/core/indexes/numeric.py](https://codecov.io/gh/pandas-dev/pandas/pull/23538/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvaW5kZXhlcy9udW1lcmljLnB5) | `62.55% <100%> (+0.5%)` | :arrow_up: |
| [pandas/core/indexes/timedeltas.py](https://codecov.io/gh/pandas-dev/pandas/pull/23538/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvaW5kZXhlcy90aW1lZGVsdGFzLnB5) | `40% <25%> (+0.17%)` | :arrow_up: |
| [pandas/core/indexes/period.py](https://codecov.io/gh/pandas-dev/pandas/pull/23538/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvaW5kZXhlcy9wZXJpb2QucHk=) | `39.61% <33.33%> (-0.22%)` | :arrow_down: |
| [pandas/core/indexes/category.py](https://codecov.io/gh/pandas-dev/pandas/pull/23538/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvaW5kZXhlcy9jYXRlZ29yeS5weQ==) | `51.05% <50%> (-0.01%)` | :arrow_down: |
| [pandas/core/indexes/base.py](https://codecov.io/gh/pandas-dev/pandas/pull/23538/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvaW5kZXhlcy9iYXNlLnB5) | `56.1% <72.22%> (+0.16%)` | :arrow_up: |
| [pandas/core/indexes/range.py](https://codecov.io/gh/pandas-dev/pandas/pull/23538/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvaW5kZXhlcy9yYW5nZS5weQ==) | `53.84% <75%> (+0.13%)` | :arrow_up: |
| [pandas/core/indexes/datetimes.py](https://codecov.io/gh/pandas-dev/pandas/pull/23538/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvaW5kZXhlcy9kYXRldGltZXMucHk=) | `52.61% <88.88%> (+0.42%)` | :arrow_up: |
| [pandas/core/dtypes/common.py](https://codecov.io/gh/pandas-dev/pandas/pull/23538/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvZHR5cGVzL2NvbW1vbi5weQ==) | `68.9% <0%> (-1%)` | :arrow_down: |

------

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

@jreback - tests are back to clean state. Thoughts on what we should do about the cases where the public facing union methods of the subclassed indexes differ from the base? Should we just remove them and let them all match the base docstring, or should we leave the overriden union methods (not a fan of this method, but not sure of a more straightforward/clean way (i.e. not getting too meta) to only override the docstring)?
@datapythonista - any ideas on the developer.rst linting errors? I'm getting this in multiple branches and not sure how metadata should be defined here.
@ArtinSarraf sorry, my fault, I merged an old PR without a rebase, and that is breaking the build. I created #24160 to fix it. You should rerun the build once it is merged, and these errors should be gone.
@jreback - I've removed public union methods and made a couple other fixes. Any thoughts on this?
https://github.com/pandas-dev/pandas/pull/23538#discussion_r240017748
can you merge master and @TomAugspurger has some questions.
@jreback - merged master and answered @TomAugspurger 's questions.
@jreback / @jschendel / @TomAugspurger - anything else to consider?
Restarted the travis network failure.
can you merge master, just merged in addition of sort=True for other set ops. ping on green.
@jreback - tests are passing. Passed the sort parameter to all instances of `_union` and made them required arguments in those methods. Only changes I made to the tests as a result of the merge was slightly broadening the list of possible error messages in pandas/tests/reshape/test_concat.py file.



@jreback - master merged and back to green again.
@jreback - all checks have passed
@jschendel - fixed all the items you pointed out. Thanks.
@jreback - anything else to consider? Particularly regarding this 
https://github.com/pandas-dev/pandas/pull/23538#discussion_r264484749?
> On master, the union of an Int64Index and UInt64Index is Int64

It looks like this is usually the case, but there's some ambiguity on `master` where it depends on the values being held by each index, specifically where the overlap is in terms of the range of valid values for each index type.   There's also some inconsistencies in that sometimes the order of the union appears to impact the type, and sometimes the order of the values within each index appears to impact the type:

```python
In [2]: pd.UInt64Index([np.iinfo(np.uint64).max]).union(pd.Int64Index([1]))
Out[2]: UInt64Index([1, 18446744073709551615], dtype='uint64')

In [3]: pd.UInt64Index([np.iinfo(np.uint64).max]).union(pd.Int64Index([-1]))
Out[3]: Index([-1, 18446744073709551615], dtype='object')

In [4]: pd.UInt64Index([1]).union(pd.Int64Index([1]))
Out[4]: UInt64Index([1], dtype='uint64')

In [5]: pd.Int64Index([1]).union(pd.UInt64Index([1]))
Out[5]: Int64Index([1], dtype='int64')

In [6]: pd.UInt64Index([1, 2]).union(pd.Int64Index([1, 2]))
Out[6]: UInt64Index([1, 2], dtype='uint64')

In [7]: pd.UInt64Index([2, 1]).union(pd.Int64Index([1, 2]))
Out[7]: Int64Index([1, 2], dtype='int64')

In [8]: pd.UInt64Index([1]).union(pd.Int64Index([2]))
Out[8]: Int64Index([1, 2], dtype='int64')
```

> I think if possible (no overflow) we want an Int64 index there.

I'd  be fine with this, but to expand a little I think we'd want to have fallbacks in this order of preference:
  1. `Int64Index` if possible  (e.g. `[4]`-`[8]` above)
  2. `UInt64Index` if possible (e.g. `[2]` above)
  3. `Index` otherwise (e.g. `[3]` above)

I think this is the implementation that's most consistent with the existing behavior on `master` that fixes the noncommutative corner cases on `master` .
While it's a case that would probably never happen, conceptually the idea demonstrated below feels wrong

```
idx1 = pd.Int64Index([0, 1, 2, 3])
idx2 = pd.UInt64Index([np.iinfo(np.uint64).max])
assert idx1.union(idx2).dtype == (idx1 - 1).union(idx2).dtype  # -> Fails
```

I would think a consistent return type from all operations would be preferred.
I also like the idea of type stability. We need to balance that against the performance impact of (unnecessarily) casting to object dtype. It's not clear what's best here.
@TomAugspurger - while this wasn’t an explicit endorsement of the current behavior on this branch, it was at least to some degree considered here:
https://github.com/pandas-dev/pandas/pull/23538#discussion_r238915058

@jreback - any thoughts on this
will have a look over the weekend
What was the resolution of the different, but sometimes compatible dtypes like uint and int? What does np.concatenate do here?

> On Mar 20, 2019, at 20:59, ArtinSarraf <notifications@github.com> wrote:
> 
> @ArtinSarraf commented on this pull request.
> 
> In pandas/tests/indexes/test_setops.py:
> 
> > +    # This is true before this PR as well.
> +
> +    # Union with a non-unique, non-monotonic index raises error
> +    # This applies to the boolean index
> +    idx1 = idx1.sort_values()
> +    idx2 = idx2.sort_values()
> +
> +    assert idx1.union(idx2).dtype == np.dtype('O')
> +    assert idx2.union(idx1).dtype == np.dtype('O')
> +
> +
> +@pytest.mark.parametrize('idx_fact1,idx_fact2',
> +                         COMPATIBLE_INCONSISTENT_PAIRS.values())
> +def test_compatible_inconsistent_pairs(idx_fact1, idx_fact2):
> +    # GH 23525
> +    idx1 = idx_fact1(10)
> Considering the only "compatible inconsistent pair" currently is RangeIndex/Int64Index it seems that indexes/test_range is much more thorough then this test I have here. It would make more sense to remove the test here.
> 
> However, I find this test to be convenient since it is very clear what pairs make up special cases, and adds very little overhead.
> 
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub, or mute the thread.