[WIP] Test (and more fixes) for duplicate indices with concat
State: closed

- [ ] closes #31308, #36263
- [ ] tests added / passed
- [ ] passes `black pandas`
- [ ] passes `git diff upstream/master -u -- "*.py" | flake8 --diff`
- [ ] whatsnew entry

Follow up to #38654, currently WIP.

It's a work in progress because it turns out union and intersection act differently for different index types. `intersection` seems mostly normal, except for `IntervalIndex` #38743), while `union` is sometimes a set union (for `Index`) and sometimes keeps duplicates (for many `Index` subclasses, though order matters sometimes).

This effects `concat` in `get_objs_combined_axis`. Once I figured out where the problems were coming from, I figured I could maybe side step this by adding checks to `get_objs_combined_axis` for equality and uniqueness. All checks pass `reshape` on my machine, but I'm done working on this for the day, so I'll rely on CI to do the checks on the rest of the codebase.

------------------------

This test could be expanded with:

* More types of indexes (there is no `object` index currently)
* Checks for commutativity, i.e. results should have set-equal columns regardless of order
* Interactions with other arguments (`sort`, `axis`?)

TODO:

- [ ] Move test for `concat` where duplicates don't error
- [ ] Decide whether error should be `DuplicateLabelErrors`
- [ ] Have error report which duplicates are used
- [ ] Check that error reports which duplicates are used

Labels:
name: Reshaping
description: Concat, Merge/Join, Stack/Unstack, Explode
name: Error Reporting
description: Incorrect or improved errors from pandas
name: Stale

Comments:
Hello @ivirshup! 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 2020-12-30 03:49:30 UTC
Should the errors here be `DuplicateLabelError`s?
> because it turns out union and intersection act differently for different index types

@phofl has been working on this recently, might shed some light on the intended behavior medium-long term
``union`` and ``intersection`` should both be unique, but as you mentioned this may not be the case for all Index types right now. 

Additionally to your issue #38623 also is problematic. Do you have a list of Index classes where this is not the case for union?
Put up #38834 to fix the IntervalIndex.intersection bug
> Do you have a list of Index classes where this is not the case for union?

@phofl, let me know if there is an open issue I should post this to. It seems like it's most of them except `MultiIndex` (but don't know if this depends on the "inner types"). For `Index` it depends on the order. `RangeIndex` is a bit of a different case since it should always be unique – I think.

<details>
<summary> Code used to check </summary>

```python
import pandas as pd
import numpy as np

import pandas._testing as tm

index_makers = [
    tm.makeStringIndex,
    tm.makeIntIndex,
    tm.makeUIntIndex,
    tm.makeFloatIndex,
    tm.makeDateIndex,
    tm.makeTimedeltaIndex,
    tm.makePeriodIndex,
    tm.makeMultiIndex,
    tm.makeBoolIndex,
]

records = []

union = lambda x, y: x.union(y)
intersection = lambda x, y: x.intersection(y)

# union = lambda x, y: pd.core.indexes.api._get_combined_index([x, y], intersect=False)
# intersection = lambda x, y: pd.core.indexes.api._get_combined_index([x, y], intersect=True)

for index_maker in index_makers:
    idx1 = index_maker(k=4)
    idx2 = idx1[np.array([0, 0, 1, 2, 3])]
    assert type(idx1) == type(idx2)
    rec = {"index_type": type(idx1).__name__, "index_generator": index_maker.__name__}
    
    # idx_intersect = intersection(idx1, idx2)
    # rec["intersect_len"] = len(idx_intersect)
    # rec["intersect_commutative"] = idx_intersect.sort_values().equals(intersection(idx2, idx1).sort_values())

    idx_union = union(idx1, idx2)
    rec["union_len"] = len(idx_union)
    rec["union_commutative"] = idx_union.sort_values().equals(union(idx2, idx1).sort_values())

    rec["unique_len"] = len(idx_union.unique())
    records.append(rec)

print(pd.DataFrame.from_records(records).to_markdown(index=False))
```

</details>

| index_type     | index_generator    |   union_len | union_commutative   |   unique_len |
|:---------------|:-------------------|------------:|:--------------------|-------------:|
| Index          | makeStringIndex    |           4 | False               |            4 |
| Int64Index     | makeIntIndex       |           5 | True                |            4 |
| UInt64Index    | makeUIntIndex      |           5 | True                |            4 |
| Float64Index   | makeFloatIndex     |           5 | True                |            4 |
| DatetimeIndex  | makeDateIndex      |           5 | True                |            4 |
| TimedeltaIndex | makeTimedeltaIndex |           5 | True                |            4 |
| PeriodIndex    | makePeriodIndex    |           5 | True                |            4 |
| MultiIndex     | makeMultiIndex     |           4 | True                |            4 |
| Index          | makeBoolIndex      |           4 | False               |            2 |


This is using `1.3.0.dev0+218.g3a066feb08`
Sorry I said something wrong, intersection should be unique, union not.
#36299 will make this consistent for every class except MultiIndex. Will look into this

```
| index_type     | index_generator    |   union_len | union_commutative   |   unique_len |
|:---------------|:-------------------|------------:|:--------------------|-------------:|
| Index          | makeStringIndex    |           5 | True                |            4 |
| Int64Index     | makeIntIndex       |           5 | True                |            4 |
| UInt64Index    | makeUIntIndex      |           5 | True                |            4 |
| Float64Index   | makeFloatIndex     |           5 | True                |            4 |
| DatetimeIndex  | makeDateIndex      |           5 | True                |            4 |
| TimedeltaIndex | makeTimedeltaIndex |           5 | True                |            4 |
| PeriodIndex    | makePeriodIndex    |           5 | True                |            4 |
| MultiIndex     | makeMultiIndex     |           4 | True                |            4 |
| Index          | makeBoolIndex      |           7 | True                |            2 |

```
This is what I am getting on top of my pr.

The bool Index case might be fishy, but you get this on master too, if the BoolIndex is sorted

```
idx1 = Index([False, False, False, True])
idx2 = Index([False, False, False, False, True])

idx_union = idx1.union(idx2)
idx_union
```

returns
```
Index([False, False, False, False, False, False, True], dtype='object')
```
which is consistent with non-Bool duplictaes on both sides like
```
idx1 = Index([0, 1, 1])
idx2 = Index([0, 1, 1, 2])

idx_union = idx1.union(idx2)
Int64Index([0, 1, 1, 1, 2], dtype='int64')
```
But not consistent if both are equal
```
idx1 = Index([0, 1, 1])
idx2 = Index([0, 1, 1])

idx_union = idx1.union(idx2)
Int64Index([0, 1, 1], dtype='int64')
```

Probably worth looking into how to avoid this when performing the outer join in union.
Why wouldn't unions have unique values?

And related to the `bool`s: I think – as you note – most of the issues here are from bugs dealing with the order of the values. I think it would be good to add a testing strategy which permutes how duplicates occur (e.g. at the start, at the end, in the middle, next to each other, combinations of these cases) to check this.
Currently running into issues with intersections of timestamp objects. They seem to be losing their `offset` field sometimes, not sure what's up with this yet.
Among others the union case is discussed here: #31326

I think a logical interpretation of a union is, that the union is the smallest index, so that both indexes are part of this index. 

Currently the union is handled differently based on if the indexes are monotonic or not. My pr aims to handle both cases the same and resort afterwards, this would leave us there with more consistency. The unwanted duplicates described above can be solved thereafter I think.
This PR closes #36263 which is milestoned 1.2.1 whereas this PR is milestoned 1.3. what's the best way to resolve this inconsistency?
this for 1.3 
This pull request is stale because it has been open for thirty days with no activity. Please update or respond to this comment if you're still interested in working on this.
@ivirshup closing as stale. ping if you want to continue.