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

Add step plot #27

Merged
merged 1 commit into from
Jun 24, 2019
Merged

Add step plot #27

merged 1 commit into from
Jun 24, 2019

Conversation

gjeusel
Copy link

@gjeusel gjeusel commented May 30, 2019

Hello there ! I just love this project, here is my first PR.

I haven't found any contribution guideline, tell me how I can help ! ;)

@PatrikHlobil
Copy link
Owner

@gjeusel : Hi,

great to head that you like working with Pandas Bokeh. I find it amazing that you added a completely new plottype !!! thanks a lot.

There is no contribution guideline, because I just started the project on my own and didn't think that anyone would like to contribute to it 😄. In my opinion, there should be 3 things for a successfull PR:

  1. successfull Code Review
  2. updated documentation (which is still in the moment the README.md file)
  3. addition of tests for the plot (the tests here are very high-level, i.e. I basically use the examples in the documentation and just test that they run successfully).

The development was not test-driven, which you can see in the code right now and I know that it needs a huge amount of refactoring 😞 , but I barely have time to even maintain the project with changes happening in Pandas and Bokeh.

Did you already run the tests (just Tests/test_PandasBokeh.py with pytest is enought)?

Cheers, Patrik

Copy link
Owner

@PatrikHlobil PatrikHlobil left a comment

Choose a reason for hiding this comment

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

Please consider changing the docstring as marked. Furthermore, please:

  • add the example in the README.md documentation (I would add it after the lineplot, because it is so similar)
  • add a test for the step plot in Tests/test_PandasBokeh.py

Thanks, Patrik

Either the location or the label of the columns to be used.
By default, it will use the remaining DataFrame numeric columns.
**kwds
Keyword arguments to pass on to :meth:`pandas.DataFrame.plot_bokeh`.
Copy link
Owner

Choose a reason for hiding this comment

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

Keyword arguments to pass on to :meth:pandas.DataFrame.plot_bokeh and further to bokeh.plotting.figure.step

y : int, str, or list of them, optional
The values to be plotted.
Either the location or the label of the columns to be used.
By default, it will use the remaining DataFrame numeric columns.
Copy link
Owner

Choose a reason for hiding this comment

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

maybe also add the mode keyword here, because this seems to be very specific and important for a stepplot.

The following example shows the populations for some animals
over the years.

>>> df = pd.DataFrame({
Copy link
Owner

Choose a reason for hiding this comment

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

Nice example

@PatrikHlobil
Copy link
Owner

@gjeusel : Sorry for replying so late. I already answered you a week ago, but apparently my message was not sent :(. I will now merge your request and also add your Name to the CONTRIBUTORS.md file. Do you want to have your clear name as a contributor or your alias?

@PatrikHlobil PatrikHlobil merged commit ab3b4d2 into PatrikHlobil:master Jun 24, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants