Optionally disallow duplicate labels
State: closed

This adds a property to NDFrame to disallow duplicate labels. This fixes a vexing issue with using pandas for ETL pipelines, where accidentally introducing duplicate labels can lead to confusing downstream behavior (e.g. `NDFrame.__getitem__` not reducing dimensionality).

When set (via the construction with `allow_duplicate_labels=False` or afterward via `.allows_duplicate_labels=False`), the presence of duplicate labels causes a DuplicateLabelError exception to be raised:

```pytb
In [3]: df = pd.DataFrame({"A": [1, 2]}, index=['a', 'a'], allow_duplicate_labels=False)
---------------------------------------------------------------------------
DuplicateLabelError                       Traceback (most recent call last)
<ipython-input-3-1c8833763dfc> in <module>
----> 1 df = pd.DataFrame({"A": [1, 2]}, index=['a', 'a'], allow_duplicate_labels=False)

~/sandbox/pandas/pandas/core/frame.py in __init__(self, data, index, columns, dtype, copy, allow_duplicate_labels)
    493
    494         NDFrame.__init__(
--> 495             self, mgr, fastpath=True, allow_duplicate_labels=allow_duplicate_labels
    496         )
    497

~/sandbox/pandas/pandas/core/generic.py in __init__(self, data, axes, copy, dtype, allow_duplicate_labels, fastpath)
    202
    203         if not self.allows_duplicate_labels:
--> 204             self._maybe_check_duplicate_labels()
    205
    206     def _init_mgr(self, mgr, axes=None, dtype=None, copy=False):

~/sandbox/pandas/pandas/core/generic.py in _maybe_check_duplicate_labels(self, force)
    240         if force or not self.allows_duplicate_labels:
    241             for ax in self.axes:
--> 242                 ax._maybe_check_unique()
    243
    244     @property

~/sandbox/pandas/pandas/core/indexes/base.py in _maybe_check_unique(self)
    540             # TODO: position, value, not too large.
    541             msg = "Index has duplicates."
--> 542             raise DuplicateLabelError(msg)
    543
    544     # --------------------------------------------------------------------

DuplicateLabelError: Index has duplicates.
```

This property is preserved through operations (using `_metadata` and `__finalize__`).

```pytb
In [4]: df = pd.DataFrame(index=['a', 'A'], allow_duplicate_labels=False)

In [5]: df.rename(str.upper)
---------------------------------------------------------------------------
DuplicateLabelError                       Traceback (most recent call last)
<ipython-input-5-17c8fb0b7c7f> in <module>
----> 1 df.rename(str.upper)
...

~/sandbox/pandas/pandas/core/indexes/base.py in _maybe_check_unique(self)
    540             # TODO: position, value, not too large.
    541             msg = "Index has duplicates."
--> 542             raise DuplicateLabelError(msg)
    543
    544     # --------------------------------------------------------------------

DuplicateLabelError: Index has duplicates.

```

---

### API design questions

- [ ] Do we want to be positive or negative?

```python
pd.Series(..., allow_duplicate_labels=True/False)
pd.Series(..., disallow_duplicate_labels=False/True)
pd.Series(..., require_unique_labels=False/True)
```

- [ ] In my proposal, the argument is different from the property. Do we like that? The rational is the argument is making a statement about what *to* do, while the property is making a statement about what *is* allowed.
```python
In [7]: s = pd.Series(allow_duplicate_labels=False)

In [8]: s.allows_duplicate_labels
Out[8]: False
```

- [ ] I'd like a method-chainable way of saying "duplicates aren't allowed." Some options

```python
# two methods.
s.disallow_duplicate().allow_duplicate()
```

I dislike that in combination with `.allows_duplicates`, since we'd have a property and a method that only differ by an `s`. Perhaps something like

```python
s.update_metdata(allows_duplicates=False)
```

But do people know that "allows_duplicates" is considered metadata?

---

TODO:

- [ ] many, many more tests
- [ ] Propagate metadata in more places (groupby, rolling, etc.)
- [ ] Backwards compatibility in `__finalize__` (we may be changing when we call it, and what we pass)

*Apologies for using the PR for discussion, but we needed a way to compare this to https://github.com/pandas-dev/pandas/pull/28334.*

Labels:
name: API Design
name: metadata
description: _metadata, .attrs

Comments:
I think this is ready for an initial review, to decide if this direction / behavior is OK. I'd recommend starting with the new docs section for the behavior, and `NDFrame.__finalize__` for the implementation.

cc @jorisvandenbossche @jreback @jbrockmendel @jschendel @datapythonista @ others I'm sure.
This looks very useful, thanks for the work on this. I can't think of a better API, so happy with your proposal.

Just wondering if in the future we may want `allow_duplicate_values=False` by default, but that's surely something to leave for later and keep backward-compatibility here.
> Just wondering if in the future we may want allow_duplicate_values=False by default

Yeah I think that's a possibility. But yes let's leave it for later :)
I'm +1 on this.

A couple passing comments:
- I found it a little bit confusing that the constructor parameter is `allow_duplicate_labels` but the attribute name is _slightly_ different in `allows_duplicate_labels`.  My preference here would be to make these the same.
- Is there any utility in making this configurable at the `axis` level, i.e. allow a dataframe to have duplicate index labels but not have duplicate column labels?
   - I don't think this is terribly important but anticipate that we'll get a user request for that feature, so probably best to bring this up now.

One thing I noticed is that this doesn't appear to validate when you directly assign a new index:
```python
In [2]: s = pd.Series(list('abc'), allow_duplicate_labels=False)

In [3]: s.index = [0, 0, 0]

In [4]: s
Out[4]: 
0    a
0    b
0    c
dtype: object

In [5]: s.allows_duplicate_labels
Out[5]: False
```
Once a kwarg is in the Series constructor it can be difficult to get it out (see: fastpath).  Could we put this in stealth mode for a while with something like:

```
ser = pd.Series(call_constructor_like_always)
ser._allows_duplicate_labels = False
```

Not a strong opinion, just thinking out loud.
Just out of curiosity why do you think adding this as a keyword argument to the Series / DataFrame constructor is the right approach? Would that subsequently guard against doing something like `ser.index = list("aaa")` after construction?
> I found it a little bit confusing that the constructor parameter is allow_duplicate_labels but the attribute name is slightly different in allows_duplicate_labels. My preference here would be to make these the same.

I share this concern. Do others? After using it for a few days I'm getting used to it, but still need to stop and think.

> Is there any utility in making this configurable at the axis level, i.e. allow a dataframe to have duplicate index labels but not have duplicate column labels? 

Yes. My backwards-compatible plan here is to have the setter allow `True/False/index/columns`.

```
* True : allows duplicates everywhere
* False : allows duplicates nowhere
* index : allows duplicates in the index, but not in the columns
* columns : allows duplicates in the columns, but not in the index.
```

Code-wise, it's not much effort to support. It just makes the logic a bit more complicated, so I'm holding off for now.

> Once a kwarg is in the Series constructor it can be difficult to get it out (see: fastpath). Could we put this in stealth mode for a while with something like:

Do we anticipate needing to remove this? Certainly, it can be marked as experimental if there's trepidation.

> Just out of curiosity why do you think adding this as a keyword argument to the Series / DataFrame constructor is the right approach?

Initially, I was thinking it'd be a property on Index. I think that's more natural, since it's making a statement about the Index. But users interact more with Series / DataFrame, so I think it's more ergonomic to place it there.

After implementing things, putting it on NDFrame is nice because we have `NDFrame.__finalize__` If it were on the index, things would be a be trickier.
> Do we anticipate needing to remove this? Certainly, it can be marked as experimental if there's trepidation.

I don't anticipate needing to remove this, but I _do_ anticipate other conceptually similar flags being implemented.  In that scenario, I don't like the idea of all of these becoming kwargs in the Series constructor.
I'm seeing some strange mypy issues with these changes. Lots of

```
pandas/tests/test_strings.py:206: error: List item 0 has incompatible type "Type[Series]"; expected "Type[PandasObject]"
pandas/tests/test_strings.py:240: error: List item 0 has incompatible type "Type[Series]"; expected "Type[PandasObject]"
...
```

L206 is `@pytest.mark.parametrize("box", [Series, Index])`.

In fact, I'm seeing these mypy errors with just the following diff from master

```diff
diff --git a/pandas/core/frame.py b/pandas/core/frame.py
index ef4e3e064d..7445951536 100644
--- a/pandas/core/frame.py
+++ b/pandas/core/frame.py
@@ -407,6 +407,7 @@ class DataFrame(NDFrame):
         columns: Optional[Axes] = None,
         dtype: Optional[Dtype] = None,
         copy: bool = False,
+        allow_duplicate_labels: bool = True,
     ):
         if data is None:
             data = {}
diff --git a/pandas/core/generic.py b/pandas/core/generic.py
index d59ce8db9b..64a38ca4eb 100644
--- a/pandas/core/generic.py
+++ b/pandas/core/generic.py
@@ -207,6 +207,7 @@ class NDFrame(PandasObject, SelectionMixin):
         dtype: Optional[Dtype] = None,
         attrs: Optional[Mapping[Optional[Hashable], Any]] = None,
         fastpath: bool = False,
+        allow_duplicate_labels: bool = True,
     ):
 
         if not fastpath:
diff --git a/pandas/core/series.py b/pandas/core/series.py
index 3e9d3d5c04..8d6beb42c4 100644
--- a/pandas/core/series.py
+++ b/pandas/core/series.py
@@ -203,7 +203,7 @@ class Series(base.IndexOpsMixin, generic.NDFrame):
     # Constructors
 
     def __init__(
-        self, data=None, index=None, dtype=None, name=None, copy=False, fastpath=False
+        self, data=None, index=None, dtype=None, name=None, copy=False, fastpath=False, allow_duplicate_labels=True,
     ):
 
         # we are called internally, so short-circuit
```


Any ideas what's going wrong (perhaps @simonjayhawkins or @WillAyd have a guess)?
What version of mypy / pytest are you using? Seems strange for that to pop up if that is your only diff
pytest 5.2.1
mypy 0.720


Indeed, it's quite strange. Opened https://github.com/pandas-dev/pandas/pull/29205 to add explicit types to the tests that were failing.
on master

`reveal_type([Series, Index])` gives `Revealed type is 'builtins.list[builtins.type*]'`

with the changes in this PR..

`Revealed type is 'builtins.list[def (data: Any =, Any =, Any =, name: Any =, Any =, Any =, **Any) -> pandas.core.base.PandasObject]'`
same inferred type with mypy 0.730 and 0.740
@TomAugspurger status of this
Still interested in working on it, but not happening for for 1.0.

On Wed, Jan 1, 2020 at 11:21 AM Jeff Reback <notifications@github.com>
wrote:

> @TomAugspurger <https://github.com/TomAugspurger> status of this
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub
> <https://github.com/pandas-dev/pandas/pull/28394?email_source=notifications&email_token=AAKAOIWMFJXLSIRR6U3C5D3Q3TGKRA5CNFSM4IVZNIOKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEH5JALQ#issuecomment-570069038>,
> or unsubscribe
> <https://github.com/notifications/unsubscribe-auth/AAKAOISNENLHU2JZMPFPUC3Q3TGKRANCNFSM4IVZNIOA>
> .
>

Close for now?
Another option: instead of an attribute on NDFrame, could make 
Series/NDArray
Series/DataFrame subclasses.  Then instead of worrying about `__finalize__`, just need to worry about `_constructor`.

Also might be able to simplify things by doing checks in `set_axis`?
> Series/NDArray subclasses

What would the user API for that look like?
> What would the user API for that look like?

Woops, meant Series/DataFrame subclasses.
I wouldn't want to require a subclass for this. They don't compose especially well with eachother, and I think this is something that's in principle useful for dataframe instances.
I've rebased & force pushed a slimmed down version of this. It's just the bare minimum to get things working for construction, validation, and propagation for places where `__finalize__` is already called with a single ndframe.

Notably, I'm not calling `__finalize__` in any new places, so there are many cases that drop the `allows_duplicate_labels` property. If we're OK with a half-baked implementation then I think we can merge the minimal version and propagate things as followups.

Some construction timings: Slight slowdown in the default case, which I'll investigate.

```python
import pandas as pd
import numpy as np

%timeit pd.Series(dtype=float)
215 µs ± 7.15 µs per loop (mean ± std. dev. of 7 runs, 1000 loops each)

%timeit pd.Series(dtype=float, allows_duplicate_labels=False)
225 µs ± 5.98 µs per loop (mean ± std. dev. of 7 runs, 1000 loops each)

%timeit pd.DataFrame(dtype=float)
236 µs ± 5.68 µs per loop (mean ± std. dev. of 7 runs, 1000 loops each)

%timeit pd.DataFrame(dtype=float, allows_duplicate_labels=False)
245 µs ± 3.88 µs per loop (mean ± std. dev. of 7 runs, 1000 loops each)

a = np.arange(10000)

%timeit pd.Series(dtype=float, index=a)
129 µs ± 3.49 µs per loop (mean ± std. dev. of 7 runs, 10000 loops each)

%timeit pd.Series(dtype=float, index=a, allows_duplicate_labels=False)
347 µs ± 9.52 µs per loop (mean ± std. dev. of 7 runs, 1000 loops each)

%timeit pd.DataFrame(index=a, dtype=float)
178 µs ± 4.16 µs per loop (mean ± std. dev. of 7 runs, 10000 loops each)

%timeit pd.DataFrame(index=a, dtype=float, allows_duplicate_labels=False)
397 µs ± 10.3 µs per loop (mean ± std. dev. of 7 runs, 1000 loops each)

%timeit pd.DataFrame(index=a, dtype=float, allows_duplicate_labels=False)
389 µs ± 5.85 µs per loop (mean ± std. dev. of 7 runs, 1000 loops each)
```


Probably out of scope here, but it'd be nice if we had a restriction that ensured that `df.index.names` doesn't intersection `df.columns`, and similarly that `df.columns.names` doesnt intersect `df.index`.
`concat` already called `__finalize__` so handling it here: https://github.com/pandas-dev/pandas/pull/28394/commits/f1e59326cef40dad578ca3c0d8133ce0e88470cd

Do people have thoughts on how to proceed? In particular, are we comfortable shipping something like this for 1.1 when some (many) methods don't call `__finalize__` and so the `allows_duplicate_labels` attribute is dropped?
> > I found it a little bit confusing that the constructor parameter is allow_duplicate_labels but the attribute name is slightly different in allows_duplicate_labels. My preference here would be to make these the same.
> 
> I share this concern. Do others? After using it for a few days I'm getting used to it, but still need to stop and think.
> 
> >

I think a prefer the `allow_duplicate_labels` over the change to `allows_duplicate_labels` BUT

```
def insert(self, loc, column, value, allow_duplicates=False)
```

so `allow_duplicates` may be better for consistency and be less confusing for users, even though less explicit.

Also a question on the API regarding insert.

```
>>> df = pd.DataFrame(
...     {"A": [0, 1, 2, 3]}, index=["x", "y", "X", "Y"], allows_duplicate_labels=False
... )
>>> df
   A
x  0
y  1
X  2
Y  3
>>>
>>>
>>> df.insert(1, "A", 42)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "C:\Users\simon\pandas\pandas\core\frame.py", line 3591, in insert
    self._mgr.insert(loc, column, value, allow_duplicates=allow_duplicates)
  File "C:\Users\simon\pandas\pandas\core\internals\managers.py", line 1149, in insert
    raise ValueError(f"cannot insert {item}, already exists")
ValueError: cannot insert A, already exists
>>>
>>> df.insert(1, "A", 42, allow_duplicates=True)
>>>
>>> df
   A   A
x  0  42
y  1  42
X  2  42
Y  3  42
>>>
```

where we have `ValueError: cannot insert A, already exists` do we want to change this to `DuplicateLabelError` - this will be a api breaking change

and when `allow_duplicates=True` should this override the DataFrame property, or should we have a ValueError: allow_duplicates should not be supplied ...
> so `allow_duplicates` may be better for consistency and be less confusing for users, even though less explicit.

or maybe change insert kwarg from `allow_duplicates` to `allow_duplicate_labels` with deprecation of old kwarg.

> so allow_duplicates may be better for consistency and be less confusing for users, even though less explicit.

Thanks, I wasn't aware of that argument to `.insert`. I'm going to advocate pretty firmly that *labels* be included in the argument name. Otherwise it's unclear whether you're talking disallowing duplicates in the values or the index.

I don't have a strong preference between `allows_duplicate_labels` and `allow_duplicate_labels`, but after working with it for a while, I'm slightly in favor of having the keyword match the name of the property.

> where we have ValueError: cannot insert A, already exists do we want to change this to DuplicateLabelError - this will be a api breaking change

Off the top of my head, I think we just keep these independent for now. I don't know that `.insert` could ever be made to use this new `.allows_duplicate_labels` since insert is inplace, and this PR only checks in `__finalize__` (i.e. after the inplace mutation is done).
Few general responses / questions:

- +1 on using the same name for keyword and attribute
- I am also OK with merging this as "experimental" (so without ensuring it is propagated in all places). One slight worry here is about how scalable it is to start checking for many different `method` values in `__finalize__`.
- When `attrs` was being added (https://github.com/pandas-dev/pandas/pull/29062), it was mentioned that this would help adding metadata like the one in this PR. But now it doesn't seem to use `attrs` ? (or is this for a similar reason that we afterwards left `Series.name` also out of `attrs` ?)

> I don't know that `.insert` could ever be made to use this new `.allows_duplicate_labels` since insert is inplace, and this PR only checks in `__finalize__` (i.e. after the inplace mutation is done).

Can you expand a bit on this? Is this true in general when doing inplace methods or with `inplace=True` ? 





> One slight worry here is about how scalable it is to start checking for many different method values in __finalize__.

Yeah. We do have a benchmark tracking this at https://pandas.pydata.org/speed/pandas/#finalize.Finalize.time_finalize_micro. My hope is that the extra cost of a few `if` branches is acceptable for the default case of allowing duplicate labels, but I'll keep an eye on it.

My primary performance concern is on `__finalize__` being called multiple times unnecessarily, when one public API calls another public API. That's a problem that exists today, but will be magnified if `__finalize__` becomes more expensive. We can avoid it by having an internal version that doesn't call `__finalize__` which is wrapped by a public, version that does call `__finalize__`.

> But now it doesn't seem to use attrs ? (or is this for a similar reason that we afterwards left Series.name also out of attrs ?)

Yeah, quite similar to `name`. After thinking on it more, `.attrs` feels like a place for users to store information (descriptions of columns, etc.) and I don't think we should be stuffing pandas-specific stuff into it.

> Can you expand a bit on this? Is this true in general when doing inplace methods or with inplace=True ?

The way `DataFrame.insert` is written, it operates inplace on the BlockManager.

```python
self._mgr.insert(loc, column, value, allow_duplicates=allow_duplicates)
```

But `allows_duplicate_labels` is implemented on `NDFrame`, via a check for duplicates in `__finalize__`, i.e. *after* the operation has been performed. Any *inplace* operation that introduces a duplicate will leave the dataframe in an inconsistent state: a DataFrame with `allows_duplicate_labels=False` but duplicates in one or more axes.

At the very least, I can update `DataFrame.insert` to disallow things like `.insert(allow_duplicate=True)` on a DataFrame with `allows_duplicate_labels=False`.
> Any inplace operation that introduces a duplicate will leave the dataframe in an inconsistent state

And do we know of other methods that can be done `inplace` and can introduce duplicates? For example `set_index` ? 

And what happens with indexing? Eg doing something like `df.loc[['A', 'A']]` can introduce duplicate labels from a dataframe that originally didn't have (I suppose this might end up calling `take`, in which case it should be covered)
Will check on other inplace methods.

> And what happens with indexing?

It will ideally propagate. As of now some of the loc / getitem methods call finalize, and some don't.