Skip to content

Change the description of Rectangle's xy parameter #17008

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Apr 3, 2020
Merged

Change the description of Rectangle's xy parameter #17008

merged 3 commits into from
Apr 3, 2020

Conversation

SidharthBansal
Copy link
Contributor

@SidharthBansal SidharthBansal commented Apr 2, 2020 •
edited
Loading

PR Summary

matplotlib.patches.Rectangle's first parameter, xy, is previously only documented for data plotting functions in which xy means left and bottom coordinates. However, in imshow(), the vertical axis's direction is inverted, and xy means left and top coordinates. See #15401.

In this PR, a fix is made to explain xy as starting coordinates so as to make it suitable for both cases
Closes #15433

PR Checklist

  • Has Pytest style unit tests
  • Code is Flake 8 compliant
  • New features are documented, with examples if plot related
  • Documentation is sphinx and numpydoc compliant
  • Added an entry to doc/users/next_whats_new/ if major new feature (follow instructions in README.rst there)
  • Documented in doc/api/api_changes.rst if API changed in a backward-incompatible way

@SidharthBansal
Change the description of Rectangle's xy parameter
677a57c 
matplotlib.patches.Rectangle's first parameter, xy, is previously only documented for data plotting functions in which xy means left and bottom coordinates. However, in imshow(), the vertical axis's direction is inverted, and xy means left and top coordinates. See matplotlib#15401.

In this PR, a fix is made to explain xy as starting coordinates so as to make it suitable for both cases
Closes matplotlib#15433
Taken things from matplotlib#15433.
@SidharthBansal
Copy link
Contributor Author

@timhoffm @anntzer @story645 @tacaswell kindly review and suggest changes if any. I am just trying to complete some pending PRS from which contributors left. This will help me to learn about mpl :-)
Thanks

@SidharthBansal
Copy link
Contributor Author

I am not able to add reviewers' from lefthand side topmost panel. Can anyone give me access to add reviewers if possible?
Thanks
Sidharth

@QuLogic
Copy link
Member

QuLogic commented Apr 3, 2020

I'm not sure I agree with this change, as what constitutes the anchor point of the rectangle?

@SidharthBansal
Copy link
Contributor Author

SidharthBansal commented Apr 3, 2020 via email

@SidharthBansal
Copy link
Contributor Author

SidharthBansal commented Apr 3, 2020 •
edited
Loading

Kindly check #15401 (comment) and #15433

@timhoffm
Copy link
Member

timhoffm commented Apr 3, 2020 •
edited
Loading

Whether xy is really the bottom left depends on the direction of the axis and the sign of width and height.

IMHO, it's correct to call it anchor point, but this needs more context:

The rectangle extends from xy by width in x-direction and by height in y-direction.

or

The rectangle extends from xy[0] to xy[0] + width and from xy[1] to xy[1] + height.

or similar. Please find a suitable wording yourself.

@SidharthBansal
Copy link
Contributor Author

Done! Thanks for a detailed explanation.

@SidharthBansal
Copy link
Contributor Author

SidharthBansal commented Apr 3, 2020 via email

@story645
Copy link
Member

story645 commented Apr 3, 2020 •
edited
Loading

You need to fix the CI error (which is a flake error) before it can be merged though

@SidharthBansal
Copy link
Contributor Author

@story645 I have rectified the flake error. Kindly merge.

@story645 story645 merged commit a451e2c into matplotlib:master Apr 3, 2020
@QuLogic QuLogic added this to the v3.3.0 milestone Apr 3, 2020
@timhoffm timhoffm mentioned this pull request Apr 4, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@story645 story645 story645 approved these changes

Assignees
No one assigned
Labels
Documentation
Projects
None yet
Milestone
v3.3.0
Development

Successfully merging this pull request may close these issues.
4 participants
@SidharthBansal @QuLogic @timhoffm @story645