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

added Stairs to basic plot types #25924

Closed
wants to merge 2 commits into from
Closed

Conversation

mariamalykh
Copy link

@mariamalykh mariamalykh commented May 19, 2023

PR summary

PR checklist

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Thank you for opening your first PR into Matplotlib!

If you have not heard from us in a while, please feel free to ping @matplotlib/developers or anyone who has commented on the PR. Most of our reviewers are volunteers and sometimes things fall through the cracks.

You can also join us on gitter for real-time discussion.

For details on testing, writing docs, and our review process, please see the developer guide

We strive to be a welcoming and open project. Please follow our Code of Conduct.

@jklymak
Copy link
Member

jklymak commented May 19, 2023

Thanks for this - however, plot-types is purposely not to be exhaustive - if I were to do anything, it would be to replace step with stairs, but I don't think we should have both as they are too similar.

@mariamalykh
Copy link
Author

Yes, but I can't do it with my access. Can you delete step plot type?

@story645
Copy link
Member

story645 commented May 21, 2023

@mariamalykh you should be able to delete it locally on your working branch and push that change up. If you do, then the file will be deleted once your PR is merged.

Though I disagree with deleting it & would like plot types to be a bit more comprehensive with respect to the plotting methods - imshow and pcolormesh also produce similar plots and the difference is in how the data is structured just like stairs vs. step and imshow and pcolormesh are in the plot gallery.

@mariamalykh
Copy link
Author

@mariamalykh you should be able to delete it locally on your working branch and push that change up. If you do, then the file will be deleted once your PR is merged.

Though I disagree with deleting it & would like plot types to be a bit more comprehensive with respect to the plotting methods - imshow and pcolormesh also produce similar plots and the difference is in how the data is structured just like stairs vs. step and imshow and pcolormesh are in the plot gallery.

maybe that's why we should accept the 1st variant of the request, it has passed all the autotests

@jklymak
Copy link
Member

jklymak commented May 22, 2023

Stairs and step are the same, just with different ways of specifying the step edges. The imshow and pcolormesh are similar, but pcolormesh is much more flexible on the spacing and location of the x and y grids. If someone proposed adding pcolor or pcolorfast to the basic plot types I would also object that they are the same as pcolormesh and are not needed. In my opinion having stair and step on this page just clutters the page and is more mystifying than helpful.

I think that both here and in the method documentation both methods should refer to each other.

@rcomer
Copy link
Member

rcomer commented May 22, 2023

@mariamalykh your branch is quite far behind our repo’s main branch. The file you are trying to delete had been updated here since you created your fork. You will need to rebase to pick up those changes, and then the deletion should work.

@jklymak
Copy link
Member

jklymak commented May 22, 2023

Remember to do "git rm" or "git mv" so git knows about the change.

@rcomer rcomer linked an issue Jun 3, 2023 that may be closed by this pull request
@rcomer
Copy link
Member

rcomer commented Jun 3, 2023

There is a related PR in the cheat sheets: matplotlib/cheatsheets#112

@rcomer
Copy link
Member

rcomer commented Jun 12, 2023

@mariamalykh are you still interested in working on this? Can we help you with anything?

@rcomer rcomer marked this pull request as draft July 1, 2023 16:31
@rcomer rcomer closed this in #26233 Jul 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging this pull request may close these issues.

[Doc]: More plot types? [Doc]: Exchange step() for stairs() in the Plot types - Basic section
4 participants