10000 Added get_xmargin(), get_ymargin() and get_zmargin() and tests. by turnipseason · Pull Request #26293 · matplotlib/matplotlib · GitHub
[go: up one dir, main page]

Skip to content

Added get_xmargin(), get_ymargin() and get_zmargin() and tests. #26293

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

Merged
merged 11 commits into from
Aug 24, 2023

Conversation

turnipseason
Copy link
Contributor

PR summary

Closes #26281 by adding get_xmargin(), get_ymargin() for _AxesBase and get_zmargin() for Axes3D, as well as tests for them.

PR checklist

@rcomer
Copy link
Member
rcomer commented Aug 5, 2023

Hi @turnipseason is this one waiting for review? If so, please hit the "Ready for review" button. PRs marked as draft tend to be skipped over.

@turnipseason turnipseason marked this pull request as ready for review August 5, 2023 12:53
Copy link
Member
@rcomer rcomer 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 your work on this @turnipseason. It looks good and I just have some m 8000 inor comments.

@rcomer
Copy link
Member
rcomer commented Aug 11, 2023

Thanks for the update @turnipseason - looks good.

I just learned (on gitter) that the new methods also need adding here so that they appear in the documentation.
https://github.com/matplotlib/matplotlib/blob/main/doc/api/axes_api.rst?plain=1
https://github.com/matplotlib/matplotlib/blob/main/doc/api/toolkits/mplot3d/axes3d.rst?plain=1

For most classes this is automatic and the methods appear in alphabetical order. For Axes we add them manually so we can group similar things together.

Copy link
Member
@story645 story645 left a comment

Choose a reason for hiding this comment

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

Might be overkill to add the version added directives here, but these should be mentioned in a whats new https://matplotlib.org/devdocs/devel/coding_guide.html#new-features-and-api-changes

Co-authored-by: hannah <story645@gmail.com>
Comment on lines 1 to 2
Getters for xmargin, ymargin and zmargin
------------------------------------------------------------------
Copy link
Member
@story645 story645 Aug 23, 2023

Choose a reason for hiding this comment

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

Suggested change
Getters for xmargin, ymargin and zmargin
------------------------------------------------------------------
Getters for xmargin, ymargin and zmargin
-------------------------------------------

apparently I messed up and made it too long 🤦‍♀️ -> needs to be exact at edge

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, sorry. I haven't written these before so I didn't know! I'll commit this version, then?

Copy link
Member

Choose a reason for hiding this comment

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

By eye, that line still looks too long to me. Basically the dashes need to be the same length as the title.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I should've done the previous commit by myself instead of via the github UI because it created a merge conflict. Not sure if what I did to solve it is right, but I think it's resolved in the latest commit, with the dashes being at the same level.

8000
"""
Retrieve autoscaling margin of the x-axis.

.. versionadded:: 3.7
Copy link
Member

Choose a reason for hiding this comment

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

This would be in 3.9, as 3.7 is already out:

Suggested change
.. versionadded:: 3.7
.. versionadded:: 3.9

"""
Retrieve autoscaling margin of the y-axis.

.. versionadded:: 3.7
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
.. versionadded:: 3.7
.. versionadded:: 3.9

"""
Retrieve autoscaling margin of the z-axis.

.. versionadded:: 3.7
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
.. versionadded:: 3.7
.. versionadded:: 3.9

@story645 story645 added this to the v3.9.0 milestone Aug 24, 2023
@story645
Copy link
Member

Thank you so much for your persistence, this looks good to go. Would you like one of us to squash merge or would you like to try rebasing down to 1 commit? Either choice is fine, and we can help if you'd like to do the latter.

@turnipseason
Copy link
Contributor Author

Thank you so much for your persistence, this looks good to go. Would you like one of us to squash merge or would you like to try rebasing down to 1 commit? Either choice is fine, and we can help if you'd like to do the latter.

Glad I could help! I'm a little bit afraid to screw things up with git😅. So I think it's better if you squash merge it. I'll read more on interactive rebasing (that's what would be used, right?) and try it some other time.

@story645 story645 merged commit a63dfaf into matplotlib:main Aug 24, 2023
@story645
Copy link
Member

I'll read more on interactive rebasing (that's what would be used, right?

Or regular rebasing, yup!

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.

[ENH]: Add get_xmargin, get_ymargin, get_zmargin axes methods
5 participants
0