Skip to content
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

DOC: Expose ExcelWriter as part of the Generated API #22359

Merged
merged 13 commits into from
Sep 18, 2018

Conversation

newinh
Copy link
Contributor

@newinh newinh commented Aug 15, 2018

@jschendel jschendel added Docs IO Excel read_excel, to_excel labels Aug 15, 2018
@jschendel jschendel added this to the 0.24.0 milestone Aug 15, 2018
@codecov
Copy link

codecov bot commented Aug 15, 2018

Codecov Report

Merging #22359 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master   #22359   +/-   ##
=======================================
  Coverage   92.05%   92.05%           
=======================================
  Files         169      169           
  Lines       50733    50733           
=======================================
  Hits        46702    46702           
  Misses       4031     4031
Flag Coverage Δ
#multiple 90.46% <ø> (ø) ⬆️
#single 42.24% <ø> (ø) ⬆️
Impacted Files Coverage Δ
pandas/core/generic.py 96.44% <ø> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 68273a7...e69316f. Read the comment docs.

Copy link
Member

@WillAyd WillAyd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR! Can you also add an examples section to the docstring so people know how to use it?

I think we should also prevent the abstract properties and methods from being rendered, but copying @datapythonista for input

@newinh
Copy link
Contributor Author

newinh commented Aug 16, 2018

@WillAyd yeap i'll try to add an example:)

@jorisvandenbossche
Copy link
Member

I think we should also prevent the abstract properties and methods from being rendered

Yes, and I would also add a note in the docstring explicitly saying that none of the methods/properties are considered public? (only 'public' usage is what is in the example?)
To avoid this rendering with sphinx of all methods, you need to do something like this:

.. autosummary::
   :toctree: generated/
   :template: autosummary/class_without_autosummary.rst

   ExcelWriter

@WillAyd
Copy link
Member

WillAyd commented Aug 17, 2018

@newinh this is really shaping up! When I built your latest commit locally I was still seeing the methods and attributes we are trying to hide - were you not seeing these on your end? I think there is still something that needs to be tweaked there.

One other thing that would make this nice - the mode argument is getting added in v0.24. If you can expand your example to show how to subsequently append to an Excel file that would cover all the use cases here.

@jorisvandenbossche
Copy link
Member

When I built your latest commit locally I was still seeing the methods and attributes we are trying to hide

Looking at how the Int64Index docstring is done (there everything is hidden in previous docs: https://pandas.pydata.org/pandas-docs/version/0.22.0/generated/pandas.Int64Index.html (in new docs, you get None listed, but will open a new issue for that, let's follow here the same practice)), I think this needs to be added to the docstring (a bit ugly ...):

Attributes
----------
None

Methods
-------
None

@jorisvandenbossche
Copy link
Member

Can you also add a note in the docstring explicitly saying that none of the methods/properties are considered public?

@newinh
Copy link
Contributor Author

newinh commented Aug 18, 2018

@WillAyd Indeed, I've seen a abstract properties that shouldn't have appeared in docs page. But I thought it is a problem of my local environment. (I first built the document)

Thanks to @jorisvandenbossche, I hid it.

@newinh
Copy link
Contributor Author

newinh commented Aug 18, 2018

Added example cases and more details to note.

As you know, there may be some mistakes because of my lack of English. If so, Please let me know:)

Copy link
Member

@WillAyd WillAyd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor edits. There is also a note in the docstring to "See DataFrame.to_excel for typical usage" Can you instead create a "See Also" section that references that method?

@@ -824,8 +824,56 @@ class ExcelWriter(object):

Notes
-----
None of methods and properties are considered public.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"None of methods" -> "None of the methods"

>>> with ExcelWriter('path_to_file.xlsx') as writer:
... df.to_excel(writer)

If you want to set engine that can manipulate Excel,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this second example is better served in to_excel as it generic functionality. Would be OK if you removed here and added that to the to_excel docs in a separate change


Examples
--------
Using ExcelWriter, some settings can be added.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove this line

--------
Using ExcelWriter, some settings can be added.

Default usage.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of a "." use ":"


You can set date format or datetime format

>>> with ExcelWriter('path_to_file.xlsx',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am actually surprised that we allow date / date time formatting with ExcelWriter but don't have the same usage in to_excel - if you are up to it aligning that functionality would be ideal (separate change)

>>> with ExcelWriter('path_to_file.xlsx', engine='openpyxl') as writer:
... df.to_excel(writer)

In order to write separate DataFrames to separate sheets
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just say "To write to separate sheets in a single file:"

datetime_format='YYYY-MM-DD HH:MM:SS') as writer:
... df.to_excel(writer)

It also supports append mode.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"You can also append to an existing Excel file:"

@newinh
Copy link
Contributor Author

newinh commented Aug 20, 2018

I do not know why the Travis CI build failed. Where should i look to fix it?

Copy link
Member

@WillAyd WillAyd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The test failure is because you changed pd.ExcelWriter to ExcelWriter in the example so Python doesn't know what you are referencing. If you revert that back should be OK (couple other edits needed as commented)

>>> with ExcelWriter('output2.xlsx', engine='openpyxl') as writer:
... df2.to_excel(writer)

You can set date format or datetime format:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Delete this example (keywords here are only applicable to ExcelWriter)

... df1.to_excel(writer, sheet_name='Sheet_name_1')
... df2.to_excel(writer, sheet_name='Sheet_name_2')

If you want to set engine that can manipulate Excel,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't use ExcelWriter here - just use to_excel. Also move this example up above the one preceding it

@pep8speaks
Copy link

pep8speaks commented Aug 20, 2018

Hello @newinh! Thanks for updating the PR.

Cheers ! There are no PEP8 issues in this Pull Request. 🍻

Comment last updated on August 22, 2018 at 09:30 Hours UTC

* Also Update some examples of to_excel method
@newinh newinh force-pushed the expose-excelwriter-api branch from 05884a7 to 04b59a5 Compare August 20, 2018 18:03
You can also append to an existing Excel file:

>>> with ExcelWriter('path_to_file.xlsx', mode='a') as writer:
... df.to_excel(writer)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would add here a sheet name? (I think you typically want to add a new sheet to an existing file?)

Copy link
Contributor Author

@newinh newinh Aug 21, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems more natural to do that:)

@@ -1917,18 +1917,24 @@ def _repr_latex_(self):
... columns=['col 1', 'col 2'])
>>> df1.to_excel("output.xlsx")

If you want to set engine that can manipulate Excel,
pass keyword argument named engine. Actually
engine is automatically chosen by file extension:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion with some changed language:

To set the library that used to write the Excel file, you can pass the `engine` keyword (the default engine is automatically chosen depending on the file extension):

Copy link
Member

@jorisvandenbossche jorisvandenbossche left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor comment, looks good to me!

>>> with ExcelWriter('path_to_file.xlsx', mode='a') as writer:
... df.to_excel(writer, sheet_name='Sheet3')

.. versionadded:: 0.24.0
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this one can be removed

>>> writer.save()
>>> with pd.ExcelWriter('output.xlsx') as writer:
... df1.to_excel(writer, sheet_name='Sheet_name_1')
... df2 = df1.copy()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you do this copy outside of the context manager? On initial glance I assumed it to be some requirement with the context manager itself, though that's clearly not the case

Copy link
Member

@WillAyd WillAyd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this looks good and slightly prefer the example with append mode but am largely indifferent.
@jorisvandenbossche merge when satisfied.

@jorisvandenbossche
Copy link
Member

slightly prefer the example with append mode but am largely indifferent

@WillAyd what do you mean exactly?

@WillAyd
Copy link
Member

WillAyd commented Aug 22, 2018

@WillAyd what do you mean exactly?

Your last comment asked to remove a piece that I would slightly prefer to keep in, but not going to lose sleep over it if you think its superfluous and still want to remove 👍

@jreback jreback merged commit c44581f into pandas-dev:master Sep 18, 2018
@jreback
Copy link
Contributor

jreback commented Sep 18, 2018

thanks @newinh

@newinh newinh deleted the expose-excelwriter-api branch September 18, 2018 14:51
aeltanawy pushed a commit to aeltanawy/pandas that referenced this pull request Sep 20, 2018
Sup3rGeo pushed a commit to Sup3rGeo/pandas that referenced this pull request Oct 1, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Docs IO Excel read_excel, to_excel
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Expose ExcelWriter as part of the Generated API
6 participants