BUG: dataframe loading with duplicated columns and usecols #11823
State: closed

Fixes #11823 .

changed `usecols` from set to list in the C parser to handle duplicated columns and `usecols`. Update python parser to be consistent with C parser.


Labels:
name: Bug
name: IO CSV
description: read_csv, to_csv

Comments:
can you update

Sorry haven't had a chance, will do soon ...

Rewrote the logic using `Index` and `.get_loc`. It's still a bit tricky though, having to convert possible outputs of `get_loc` to unique indices for each duplicated column name. 

Before trying to solve the bug (if it is a bug), what is actually the desired behaviour?

Current behaviour on the example you gave:

```
In [1]: pd.read_csv(StringIO("""1,2,3"""), engine='python', header=None,
   ...:             names=['a', 'b', 'a'], usecols=['a'])
Out[1]:
   a
0  1

In [2]: pd.read_csv(StringIO("""1,2,3"""), engine='python', header=None,
   ...:             names=['a', 'b', 'a'], usecols=['a','a'])
Out[2]:
   a
0  1
```

In the example output you gave here https://github.com/pydata/pandas/pull/11882#discussion_r49555768, you changed `Out[2]` to include two columns:

```
pd.read_csv(StringIO("""1,2,3"""), engine='python', header=None, 
            names=['a', 'b', 'a'], usecols=['a','a'])
Out: 
   a  a
0  3  3
```

But I don't think this is the desired behaviour? Shouldn't it rather be something like:

```
pd.read_csv(StringIO("""1,2,3"""), engine='python', header=None, 
            names=['a', 'b', 'a'], usecols=['a','a'])
Out: 
   a  a
0  1  3
```

And I would argue that `Out[1]` (so using `usecols=['a']` should yield the same)

The desired behavior is simply to match the `engine='c'` parser, which is arguably not ideal either. For the example case with `['a', 'a']`:

```
pd.read_csv(StringIO("""1,2,3"""), engine='c', header=None, 
            names=['a', 'b', 'a'], usecols=['a','a'])
Out[2]: 
   a  a
0  3  3
```

I agree that your suggested output of `[1, 3]` would be better. Then need to fix the c parser as well ...

Well, I would rather say that the behaviour of the c parser _is_ a bug. 
In any case, both are wrong IMHO, and I find the behaviour of the c parser not any better.

And BTW, I get even another behaviour on master:

```
In [1]: pd.read_csv(StringIO("""1,2,3"""), engine='c', header=None,
   ...:             names=['a', 'b', 'a'], usecols=['a','a'])
Out[1]:
   a  a
0  1  1
```

@sxwang 

> What about throwing an error when duplicated columns exist with usecols? Or leave the python engine alone and edit only the c engine parser so that the "(b) looks like a bug" part of the original issue #11823 is addressed, but not the "(a) different" part?

I think this is indeed better

yeh I think the ideal is is to make c-parser / python parser the same for now (part b)

@jorisvandenbossche which one is indeed better?
If I leave the python parser alone for now and just edit the C parser to fix the `NaN` bug in the original issue, then we get this:

```
In [5]: pd.read_csv(StringIO("""1,2,3"""), engine='c', header=None, 
names=['a', 'a', 'b'], usecols=['a','a','b'])
Out[5]: 
   a  a  b
0  2  2  3
In [6]: pd.read_csv(StringIO("""1,2,3"""), engine='python', header=None, 
names=['a', 'a', 'b'], usecols=['a','a','b'])
Out[6]: 
   a  b
0  1  3
```

If there is consensus, I'll continue to try to fix the python parser to match the still-non-ideal c parser behavior with hopefully less complicated code.

@sxwang sorry, that was not really clear. I meant that I more like to try fixing the bug, than just make the behaviour for c and python parser the same, but still buggy. (or if that's too difficult, I think raising an error is even better as the current situation).

> yeh I think the ideal is is to make c-parser / python parser the same for now (part b)

@jreback that was actually part (a) :-) But kidding aside, I rather think we should fix the bug than just making their behaviour the same. 
But the question still is what we look like this to do.

@sxwang Personally, I think I would like to see this behaviour:

```
In [6]: pd.read_csv(StringIO("""1,2,3"""), engine='python', header=None, 
                    names=['a', 'a', 'b'], usecols=['a','a','b'])
Out[6]: 
   a  a  b
0  1  2  3

In [6]: pd.read_csv(StringIO("""1,2,3"""), engine='python', header=None, 
                    names=['a', 'a', 'b'], usecols=['a','b'])
Out[6]: 
   a  a  b
0  1  2  3
```

So once the column 'a' is specified in the `usecols` keyword, get all columns with this name, regardless of how many times it is specified in `usecols`, as the behaviour of this keyword should not depend on the order of columns specified or how many times it is specified. 
But of course this can be discussed, but to me that seems the most sensible (and predictable) thing to do.

Of course the current bug in the C parser is even worse as the behaviour in the python parser, but ideally we should fix them both at once.

can you rebase/update

Rebased.

On Sat, Mar 12, 2016 at 9:49 AM Jeff Reback notifications@github.com
wrote:

> can you rebase/update
> 
> —
> Reply to this email directly or view it on GitHub
> https://github.com/pydata/pandas/pull/11882#issuecomment-195779861.

So I don't think test coverage is sufficient here. We need these 2 work (you actually just add a single tests w/o specifying `engine` and use `self.read_csv` in the test_parsers.py). Trick is to hard code the expected frame.

```
In [1]: pd.read_csv(StringIO("""1,2,3"""), engine='c', header=None, 
   ...: names=['a', 'a', 'b'], usecols=['a','a','b'])
Out[1]: 
   a  a    b
0  2  2  NaN

In [2]: pd.read_csv(StringIO("""1,2,3"""), engine='python', header=None, 
   ...: names=['a', 'a', 'b'], usecols=['a','a','b'])
Out[2]: 
   a  a  b
0  2  2  3
```

I messed something up ... sorry, still trying to learn my way around git. Let me close and submit a new PR ...

@sxwang you don't need to open a new PR to clean it up here. If you can clean up your branch locally, just push force it your origin/dup_column_usecols branch and it will be updated here

Ok, I managed to fix the branch.

I think there are 2 distinct things to test, which are reflected in the new test I added and the comments:
(1) matching behavior with and without usecols

```
In [4]: pd.read_csv(StringIO("""1,2,3"""), names=['a', 'a', 'b'], header=None, usecols=['a', 'a', 'b'])
Out[4]: 
   a  a  b
0  2  2  3

In [5]: pd.read_csv(StringIO("""1,2,3"""), names=['a', 'a', 'b'], header=None)
Out[5]: 
   a  a  b
0  2  2  3
```

(2) matching behavior between c and python engines

```
In [6]: pd.read_csv(StringIO("""1,2,3"""), engine='c', names=['a', 'a', 'b'], header=None, usecols=['a', 'a', 'b'])
Out[6]: 
   a  a  b
0  2  2  3

In [7]: pd.read_csv(StringIO("""1,2,3"""), engine='python', names=['a', 'a', 'b'], header=None, usecols=['a', 'a', 'b'])
Out[7]: 
   a  a  b
0  2  2  3
```

I think this is the result we'd like to achieve (in all the above cases).

```
In [7]: df = pd.read_csv(StringIO("""1,2,3"""), engine='c', header=None)

In [8]: df.columns=list('aab')

In [9]: df
Out[9]: 
   a  a  b
0  1  2  3
```

@sxwang quoting your expected behaviour of above:

```
In [5]: pd.read_csv(StringIO("""1,2,3"""), names=['a', 'a', 'b'], header=None)
Out[5]: 
   a  a  b
0  2  2  3
```

IMO this should _not_ be the expected behaviour (unless it was a typo?). As you get here not the values that were in your csv file, while you just provided names for the columns (no selection of the columns).

@sxwang note that I just merged #12551; though this shouldn't affect this PR (even though it involves usecols)

@sxwang also have a look at some discussion on #12678 

can you rebase/update

I think making this have the "ideal" behavior is quite involved ... and I'm afraid I don't really have to bandwidth to work on this anymore. Maybe somebody else can take a stab at it. Should I close the PR?

@sxwang : I too have been working on fixing duplicate columns issues.  If you don't mind, I'd be happy to pick this one up / resolve this issue once my first PR for duplicate columns gets merged.

@jreback : From what I see in this discussion, I should point out that I am not the only person who has been stating that this "ideal" (which you lay out here <a href="https://github.com/pydata/pandas/pull/11882#issuecomment-198946212">here</a>) would take a lot of work / refactoring to do.  Two different contributors (@sxwang and myself) independently working on this issue have expressed this opinion.  If you really do believe your idea of lists is that simple to implement, then IMHO it should not take very long for you to do so yourself because right now, neither of us seem to see an easy path ATM.

@gfyoung thanks, I'll close this one.