-
-
Notifications
You must be signed in to change notification settings - Fork 18.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
COMPAT: Allow multi-indexes to be written to excel #10570
COMPAT: Allow multi-indexes to be written to excel #10570
Conversation
('2014','weight')]) | ||
df = pd.DataFrame(np.random.randn(10,3), columns=cols) | ||
import warnings | ||
with warnings.catch_warnings(record=True) as w: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use tm.assert_produces_warning
Is this I agree it can be annoying to not be able to remove a warning, but a keyword just for this that maybe is not needed anymore in a next release seems also a bit unnecessary |
another way to do this is to make a custom warning class (e.g. inherit from |
@@ -119,6 +119,7 @@ Other API Changes | |||
- Enable serialization of lists and dicts to strings in ExcelWriter (:issue:`8188`) | |||
- Allow passing `kwargs` to the interpolation methods (:issue:`10378`). | |||
- Serialize metadata properties of subclasses of pandas objects (:issue:`10553`). | |||
- Allow multi-indexes columns to be written to Excel with a one way serialization (:issue: `10564`). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this needs a bit more explanation (e.g. we removed this in 0.16.2 (or was it 1?)) because of the one-way serialization.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how about ?:
+- Allow ``DataFrame`` with ``MultiIndex`` columns to be written to Excel (:issue: `10564`). The original fix implemented for (:issue:`9794`) was slightly overkill as only ``DataFrame``s without indexes turned out to be broken Excel files.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
more like:
this was changed in 0.16.2 as the read-back method could not always guarantee perfect fidelity.
It is maybe not reversible with a simple BTW, the docstring of |
And, @jreback, I like the idea of having a global way to turn of this kind of UserWarnings more than special keywords all over the place (and if we want an argument for it, I would rather go with something more general, like |
hmm, ok could have Though maybe a more general |
Is this still being considered? |
can you rebase / squash? @jorisvandenbossche what do you think about |
Yeah, I'll see if I can pull in the master and squash. |
Sorry I'm bungling this at the moment. |
Ok. Got my git figured out. Hopefully tests will still pass. Thanks. |
"not yet implemented.") | ||
if not index: | ||
raise NotImplementedError("Writing to Excel with MultiIndex" | ||
" columns and no index is not yet" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you change 'and no index' to 'and with 'index=False' ' (otherwise you could understand that the dataframe has no index)
In any case, I think |
@@ -1305,11 +1309,17 @@ def to_excel(self, excel_writer, sheet_name='Sheet1', na_rep='', | |||
""" | |||
from pandas.io.excel import ExcelWriter | |||
if self.columns.nlevels > 1: | |||
raise NotImplementedError("Writing as Excel with a MultiIndex is " |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would move this entire thing to core.format.ExcelFormatter
didn't realize it was here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not exactly sure what you're looking for. Just the 'raise NotImpl...' moved into the ExcelFormatter init? or where?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this type of checking on the index should go about here. Then all of this code will be in one place.
Ok. Couldn't figure out what to do with the verbose keyword, so I improvised and copied someone else's custom warning. |
you need to still accept the |
Now other things are breaking and I don't really see why. I'm just about at the end of my rope here. |
just pass it thru, you had added it before; just put it back in |
I'm not sure what the tests were trying to catch before or if they're good, but I can make them pass by reordering the tests for valid input before testing for aliases. (seems kinda sketchy) |
Don't think this was my fault: https://travis-ci.org/pydata/pandas/jobs/75683428, it passed on the branch https://travis-ci.org/flamingbear/pandas/builds/75683397 I just pushed a no-op branch up again. Is there a way I could have scheduled travis tests without pushing the branch again? |
@jreback I think this is ready for you now. Thanks. Matt |
lgtm. @jorisvandenbossche ? |
@@ -1640,11 +1644,14 @@ class ExcelFormatter(object): | |||
inf_rep : string, default `'inf'` | |||
representation for np.inf values (which aren't representable in Excel) | |||
A `'-'` sign will be added in front of -inf. | |||
verbose: boolean, default True | |||
If True, warn user that the resulting output file may not be | |||
re-read or parsed directly by pandas. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you use normal indentation (4 spaces here)? no alignment with the 'boolean, ..' is needed. (se the other parameters)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, I can do that.
@jreback @jorisvandenbossche : Can you give me some help here? This happened before and I repushed and it fixed itself. I don't know what a ResourceError is, https://travis-ci.org/pydata/pandas/jobs/75848253, but it's still passing fine on my travis. I can keep pushing until it goes through again. Is that the best option? This happened before #10570 (comment) |
just leave for now |
alright if this one fails, I'll leave it. Thanks |
"is not yet implemented.") | ||
elif self.index and self.verbose: | ||
warnings.warn("Writing to Excel with MultiIndex columns is a" | ||
" one way serializable operation. You will not" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
here is where I mean
@flamingbear minor change, then lgtm. ping when green. |
(Even though they cannot be read back in.) Closes #10564
@jreback we're green. Thanks. |
Looks good! Thanks! |
…el-writing COMPAT: Allow multi-indexes to be written to excel
Thanks guys. |
(Even though they cannot be read back in)
Closes #10564