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/CLN: Cleanups plotting.py #8037

Merged
merged 1 commit into from
Oct 4, 2014
Merged

Conversation

sinhrks
Copy link
Member

@sinhrks sinhrks commented Aug 15, 2014

  • Some refactoring which should not affects to output.
  • Made DataFrame.plot and Series.plot should have consistent docstring except differences.

@sinhrks sinhrks mentioned this pull request Aug 15, 2014
9 tasks
@jreback jreback added this to the 0.15.0 milestone Aug 15, 2014
return plot_obj.axes[0]


df_kind = """kind : {'line', 'bar', 'barh', 'hist', 'kde', 'density', 'area', 'pie', scatter', 'hexbin'}
Copy link
Member

Choose a reason for hiding this comment

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

This is list getting rather long ..
What do you think of just doing kind : string, default 'line' ? As all possibilities are also listed underneath? (and maybe in that case put them in quotes in the list like - 'bar' : vertical bar plot, then it is also clear the those strings are the options)

@jorisvandenbossche
Copy link
Member

@sinhrks Docstrings are looking nice! Made some comments (and note that a lot of them are comments on the existing docstrings, but thought was a good opportunity to look at that too)

I did not yet really look at the actual code refactor, but the only think I was worrying a bit is: you changed the order of keyword arguments, that is right? Now you put kind second (which is very logical), before x and y. But I was wondering how many people would rely on the position of x and y? (for the others that doesn't matter I think, but for those first keyword, it does maybe)

@sinhrks
Copy link
Member Author

sinhrks commented Aug 16, 2014

Thanks, fixed the points. And agreed to hold x and y in the current arg order.

@jorisvandenbossche
Copy link
Member

@sinhrks Can you rebase this? @TomAugspurger Can you also have a look?

@sinhrks
Copy link
Member Author

sinhrks commented Sep 5, 2014

Yep, rebased.

klass_unique=series_unique, klass_note=series_note)

_shared_docs['plot'] = """
Make plots of %(klass)s with the index on the x-axis
Copy link
Contributor

Choose a reason for hiding this comment

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

Depending on the options, the index will not be on the x-axis. You can probably just leave this off and have Make plots of %(klass)s

@TomAugspurger
Copy link
Contributor

This cleanup really needed to be done, thanks @sinhrks. Just had a few inline comments. There's also a few lines in the docstring that are longer that 80 characters, but if this is a formatting thing so that it looks nice in the REPL, then no problem.

@sinhrks sinhrks force-pushed the plot_cln branch 3 times, most recently from a7251d5 to de6d161 Compare September 6, 2014 14:11
@jreback
Copy link
Contributor

jreback commented Sep 14, 2014

@sinhrks pls rebase

@TomAugspurger
Copy link
Contributor

@sinhrks I can merge once you get a chance to rebase. Let me know.

@jreback
Copy link
Contributor

jreback commented Sep 26, 2014

@sinhrks status?

@TomAugspurger
Copy link
Contributor

@sinhrks will you have a chance to do this in the next couple days? If not I can cherry pick things. I'd like to get all the vis PRs done by tomorrow.

@jreback
Copy link
Contributor

jreback commented Oct 2, 2014

@TomAugspurger let's either cherry-pick or push to 0.15.1 whatever you can do in next couple of days

@TomAugspurger TomAugspurger merged commit 53cc1b7 into pandas-dev:master Oct 4, 2014
@sinhrks sinhrks deleted the plot_cln branch November 22, 2014 09:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants