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: Fix docstring for tsplot #17814

Closed
wants to merge 3 commits into from
Closed

DOC: Fix docstring for tsplot #17814

wants to merge 3 commits into from

Conversation

TomDonoghue
Copy link

Browsing through pandas, I noticed a small plotting function who's docs were incomplete / out of date. This is a small update to the docstring for tsplot in plotting/_timeseries.

  • closes #xxxx
  • tests added / passed
  • passes git diff upstream/master -u -- "*.py" | flake8 --diff
  • whatsnew entry

ax : matplotlib axes object, optional, default=None
Axis to plot upon. If none, plots on current axis.
kwargs
Additional keywords arguments passed to plotf.
Copy link
Member

Choose a reason for hiding this comment

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

"keywords arguments" -> "keyword arguments"

@codecov
Copy link

codecov bot commented Oct 7, 2017

Codecov Report

Merging #17814 into master will decrease coverage by 0.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #17814      +/-   ##
==========================================
- Coverage   91.26%   91.24%   -0.02%     
==========================================
  Files         163      163              
  Lines       49978    49978              
==========================================
- Hits        45611    45602       -9     
- Misses       4367     4376       +9
Flag Coverage Δ
#multiple 89.04% <ø> (ø) ⬆️
#single 40.24% <ø> (-0.07%) ⬇️
Impacted Files Coverage Δ
pandas/plotting/_timeseries.py 60.73% <ø> (ø) ⬆️
pandas/io/gbq.py 25% <0%> (-58.34%) ⬇️
pandas/core/frame.py 97.74% <0%> (-0.1%) ⬇️

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 7db7f82...55bd4a5. Read the comment docs.

@codecov
Copy link

codecov bot commented Oct 7, 2017

Codecov Report

Merging #17814 into master will decrease coverage by 0.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #17814      +/-   ##
==========================================
- Coverage   91.26%   91.24%   -0.02%     
==========================================
  Files         163      163              
  Lines       49978    49980       +2     
==========================================
- Hits        45611    45604       -7     
- Misses       4367     4376       +9
Flag Coverage Δ
#multiple 89.04% <ø> (ø) ⬆️
#single 40.24% <ø> (-0.06%) ⬇️
Impacted Files Coverage Δ
pandas/plotting/_timeseries.py 60.73% <ø> (ø) ⬆️
pandas/io/gbq.py 25% <0%> (-58.34%) ⬇️
pandas/core/frame.py 97.74% <0%> (-0.1%) ⬇️
pandas/core/groupby.py 91.99% <0%> (-0.05%) ⬇️
pandas/core/indexes/datetimes.py 95.58% <0%> (ø) ⬆️
pandas/core/reshape/pivot.py 96.38% <0%> (+0.02%) ⬆️
pandas/core/indexes/datetimelike.py 97.09% <0%> (+0.2%) ⬆️

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 7db7f82...5326533. Read the comment docs.

@@ -22,16 +22,22 @@

def tsplot(series, plotf, ax=None, **kwargs):
"""
Plots a Series on the given Matplotlib axes or the current axes
Plots a Series on the given Matplotlib axes or the current axis.
Copy link
Contributor

Choose a reason for hiding this comment

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

I think that matplotlib uses the phrase "current axes", so the old way was correct here. "axis" is for the "x" or "y" axis of an axes :)

Good you change that back here and on line 33?

@TomDonoghue
Copy link
Author

@TomAugspurger :
Yep, I think you're right, sorry about that. Fixed!

@jorisvandenbossche
Copy link
Member

Not that I don't want better docstrings (@TomDonogue thanks for contributing on that part!), but shouldn't this function rather be removed/deprecated?

We don't use it internally, and it is not exposed in the pandas.plotting namespace (it is still exposed in pandas.tseries.plotting).
And I also don't think it is actually a useful function for users? (they would just do ts.plot(), and here you have to awkwardly provide an actual plotting function)

@TomDonoghue
Copy link
Author

@jorisvandenbossche
I was basically poking through pandas, looking for an easy initial PR, so other than noticing that this function is exposed through ts.plotting, I don't have much experience with it. I'm not super clear on the use case, and whether it's all that useful. I tested it out, but it does seem like an awkward function, given you have to pass in a plotting function.

Point being, if it's better to deprecate, that makes sense to me - let me know if there is anything I can / should do on my end.

@jreback
Copy link
Contributor

jreback commented Nov 12, 2017

@TomDonoghue can you deprecate this function? add a test for the deprecation & mention in the whatsnew for 0.22

@jreback jreback added the Deprecate Functionality to remove in pandas label Nov 12, 2017
@jreback
Copy link
Contributor

jreback commented Dec 2, 2017

closing as stale. if you want to re-visit this pls ping.

@jorisvandenbossche
Copy link
Member

@TomDonoghue Still welcome to do a PR to deprecate the function instead! (opened #18627 for this)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Deprecate Functionality to remove in pandas Docs Visualization plotting
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants