BUG: Excel writer doesn't handle "cols" option correctly
State: closed

closes #5427

Notes on this PR:
- This fix addresses an issue introduced in #5235 (Issue with Excel writers when column names are duplicated).
- Basically it needs to handle 2 different use cases:
  - The user uses duplicate column names
  - The user specifies a different order via the `cols` option

``` python
# Case 1
df = pd.DataFrame([[1, 2, 3], [1, 2, 3], [1, 2, 3]])

df.columns = ['A', 'B', 'B']
```

``` python
# Case 2
df = pd.DataFrame({'A': ['a', 'a', 'a'],
                   'B': ['b', 'b', 'b']})

df.to_excel('frame.xlsx', sheet_name='Sheet1', cols=['B', 'A'])

```

The proposed solution is to iterate through self.columns in the default or user supplied order. If a duplicate column name is encountered (i.e. if `df['B']` returns more than one series) then we select the first series and keep track of that index. If the duplicate column name is encountered again then we select the next available series from `df['B']` up to the last series available. If the duplicate name is encountered again then we return the last series again.

The patch is slightly kludgy. I didn't know how to check if `df[col]` contained more than one series so I used `len(self.df[col_name].columns)` in a `try:catch`. Hopefully there is something cleaner.

The change is repeated twice in the code.

There is a new test for this issue and an existing test for the previous issue.

I don't think this needs a release note since it is a fix for an issue that was never released.


Labels:

Comments:
to_csv handles this exact issue

I'll put a refenenece up

You can do almost exactly the same things, essentially you iterate on the blocks (which are dtypes separated), and use the self.column_map to map the column (which handles all of the dup issues) 

https://github.com/pydata/pandas/blob/master/pandas/core/format.py#L897

https://github.com/pydata/pandas/blob/master/pandas/core/format.py#L1176

> to_csv handles this exact issue

@jreback I had a look at that already but it didn't look to be easily transferable since `to_csv` is handling the data in a row column order. But maybe it is. I don't mind additional input.

As an aside, row/column order handling was something that I was interested in adding. In general the `to_csv` code is much simpler than the `to_excel`. I don't see any substantive reason for the Excel complexity.

maybe it was an issue of compatibility with one of the readers?

> You can do almost exactly the same things, essentially you iterate on the blocks (which are dtypes separated), and use the self.column_map to map the column (which handles all of the dup issues)

Ok, I'm wrong, that does look applicable. I'll look at it again.

ping me again when you feel this is ready

@jreback, @jtratner 

Updated with simpler fix based on the `to_csv()` implementation.

Also, is the `tm.assert_series_equal()` method supposed to act like this:

``` python
import pandas as pd
import pandas.util.testing as tm

series1 = pd.Series(['a', 'b', 'c'])
series2 = pd.Series(['d', 'e', 'f'])

tm.assert_series_equal(series1, series2)
# No exception.
```

That sent me in circles for a while because a test that should have failed didn't.

that should assert, looks like a bug in a particular part of the checking code (which was recently changed)

@jmcnamara rebase on current master and the assert_series_equal will now work correctly....that was a bug! thanks!

```
In [4]: series1 = pd.Series(['a', 'b', 'c'])

In [5]: series2 = pd.Series(['d', 'e', 'f'])

In [6]: tm.assert_series_equal(series1, series2)

AssertionError: 'a' != 'd'
```

Ok guys, thanks for that. Update includes:
- Rebased to pick up test fix.
- Fixed temp path in test cases.
- Moving frame copy out of loop (d'oh).

@jtratner looks fine, ok to merge

@jmcnamara - can you clarify one thing? I thought you said there was a test case for dup columns specified by the user? Shouldn't that test case be failing without this PR? If not, can you add a test that covers specified dupe columns?

Maybe I'm just missing something here. 

Aside from that, this looks good to me and I will merge after I hear back. 

>  can you clarify one thing? I thought you said there was a test case for dup columns specified by the user? Shouldn't that test case be failing without this PR?

Sorry for the confusion.

Initially there was an issue reported on SO where the Excel output was failing with duplicate columns.

I wrote a test and fix for that in issue #5235 and PR #5237 but inadvertently introduced a bug for swapped columns. 

This PR fixes the swapped columns bug without displacing the duplicate column fix. In fact it should fix any further issues of this kind (at least as robustly as to_csv()).

@jmcnamara could you rebase this on top of master to make sure it merges well with your other PR (that was just merged _into_ master)?

Rebase complete. Good to merge.

Thanks!