API: Warn about dups in names for read_csv
State: closed

Title is self-explanatory.

xref #17095.

Labels:
name: API Design
name: Error Reporting
description: Incorrect or improved errors from pandas
name: IO CSV
description: read_csv, to_csv

Comments:
# [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/17346?src=pr&el=h1) Report
> Merging [#17346](https://codecov.io/gh/pandas-dev/pandas/pull/17346?src=pr&el=desc) into [master](https://codecov.io/gh/pandas-dev/pandas/commit/d43aba82e218cd8187769a07c487709aa86de693?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/17346/graphs/tree.svg?token=eZ4WkYLtcO&width=650&height=150&src=pr)](https://codecov.io/gh/pandas-dev/pandas/pull/17346?src=pr&el=tree)

```diff
@@            Coverage Diff             @@
##           master   #17346      +/-   ##
==========================================
- Coverage   91.26%   91.24%   -0.02%     
==========================================
  Files         163      163              
  Lines       49776    49783       +7     
==========================================
- Hits        45426    45424       -2     
- Misses       4350     4359       +9
```

| Flag | Coverage Δ | |
|---|---|---|
| #multiple | `89.04% <100%> (ø)` | :arrow_up: |
| #single | `40.29% <57.14%> (-0.06%)` | :arrow_down: |

| [Impacted Files](https://codecov.io/gh/pandas-dev/pandas/pull/17346?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pandas/io/parsers.py](https://codecov.io/gh/pandas-dev/pandas/pull/17346?src=pr&el=tree#diff-cGFuZGFzL2lvL3BhcnNlcnMucHk=) | `95.48% <100%> (+0.02%)` | :arrow_up: |
| [pandas/io/gbq.py](https://codecov.io/gh/pandas-dev/pandas/pull/17346?src=pr&el=tree#diff-cGFuZGFzL2lvL2dicS5weQ==) | `25% <0%> (-58.34%)` | :arrow_down: |
| [pandas/core/frame.py](https://codecov.io/gh/pandas-dev/pandas/pull/17346?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvZnJhbWUucHk=) | `97.77% <0%> (-0.1%)` | :arrow_down: |

------

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

@jreback : All comments addressed, and tests are green.  PTAL
@jreback @jorisvandenbossche : ping
Fixed typo and added to deprecation list.  Will merge on green then unless told otherwise.
@gfyoung wait for @jorisvandenbossche comment (as not sure if he commented here). IIRC a comment he made that having duplicate names is *ok* .
Sure thing.  FWIW, @jorisvandenbossche agreed with your suggestion, see his comment <a href="https://github.com/pandas-dev/pandas/pull/17095#discussion_r130438096">here</a>

@jorisvandenbossche : Any comments on this PR?
@jorisvandenbossche : ping if there any additional comments
@jreback : It's been a week, and I haven't heard anything from @jorisvandenbossche .  Still wait, or can we merge this PR?
I think Joris is off on holiday. I believe he's back next week.
@TomAugspurger : Ah!  I had a feeling that that was the case (I remember seeing an email about that).  I'll wait then until he gets back.
@jorisvandenbossche : friendly ping
Can you remember me the rationale for deprecating this? 
Is it because we cannot actually handle it well?

Because I actually previously had a usecase where this proved useful (I had a non-informative column every other column, gave it the same name in `names` and then dropped the single name. But this specific case can of course easily be solved differently, by giving names like 'dummy1', 'dummy2', .. and then removing all columns that start with 'dummy')
@jorisvandenbossche : Here is what you said back in July <a href="https://github.com/pandas-dev/pandas/pull/17095#discussion_r130437981">here</a>.

Essentially, we are deprecating this behavior because `names` is a user-specified parameter, and passing in duplicate names *deliberately* only encourages buggy behavior.
@jorisvandenbossche : Any further thoughts on this?
Sorry for the slow response. 

So maybe a more general question: is it our intention to once fix `mangle_dupe_cols=True` ? (as currently actually only the default False works). If we do, I see no good reason to disallow duplicates in passed `names` vs names in the csv file.
@jorisvandenbossche : No worries!  I think you meant the other way around.  `mangle_dupe_cols=True` is fully supported at this point.  It is `mangle_dupe_cols=False` that we disabled.

The reason for us discouraging duplicates in `names` is because duplicates are generally more error-prone.  Contrary to duplicates in a file, `names` is a *deliberate* choice.

@jreback : Thoughts?
>  I think you meant the other way around

yes, of course ... :-)

> Contrary to duplicates in a file, names is a deliberate choice.

That's true. But there are many other ways to deliberately make a dataframe with duplicate columns which we don't disallow anyway.

To be clear, in general I am all for a restricted scope of capabilities/possibilties. But in this case, limiting the abilities of `name` does not actually reduce code complexity, but increases it. As the duplicates are already perfectly handled by the code, so we are introducing a special case. Therefore I was wondering whether this is actually needed (user will see that the names are mangled anyhow).
@jorisvandenbossche : True that we'll see them mangled anyhow, but why the need to add complexity to just handle them in the first place?  I added the handling for duplicate `names` in an earlier PR because it was a bug, not to enhance support.

If the user really wants to have duplicate names, they can set it themselves and reading in the file, but I don't know if we want to actively encourage setting duplicate names to a read-in `DataFrame`.
Ah, I assumed that the mangling of `names` or the names from the header of the file was done using the same code path? That's not the case ? 
(to state it another way: once we remove the deprecation in this PR, we can actually remove more code than is added in this PR?)
See #17095 : it's a not a ridiculous amount of new logic that I added, but new logic nonetheless :smile:  Also, see @jreback comment in that PR <a href="https://github.com/pandas-dev/pandas/pull/17095/files#r129816735">here</a>
@jorisvandenbossche : Any updates on this?
so the only reason we have ``mangle_dupe_columns`` is to support duplicates in the first place. I think I stated we should deprecate that argument entirely, then I would allow duplicates in ``names`` but show a ``UserWarning`` if ``names`` contains duplicates.

So pretty much allow what is happening today but with a ``UserWarning`` and reducing the path complexity a bit (removing ``mangle_dupe_columes``).

Its not an error to have duplicates in ``names`` but I guess can't disallow it entirely.
@jreback : I don't recall you saying this before.  In addition, I think there has been user interest and *not* mangling in cases when the CSV file itself contains dupe names.  That being said, if we think making the warning less harsh is a good idea, I can do that.
@jreback : I made it issue a `UserWarning` instead.  PTAL.
lgtm. you might want to note in the doc-string the same warning. 
> lgtm. you might want to note in the doc-string the same warning.

Sounds good.  I'll quickly add that.
@jreback : All is green.  PTAL.
@jreback : All is green.  PTAL.
thanks @gfyoung I think fine for now, we can always revisit if needed.