Skip to content

Fix axes vlines and hlines using wrong coordinates #25324

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 1 commit into from
Apr 19, 2023
Merged

Fix axes vlines and hlines using wrong coordinates #25324

merged 1 commit into from
Apr 19, 2023

Conversation

robrighter
Copy link
Contributor

@robrighter robrighter commented Feb 25, 2023 •
edited by oscargus
Loading

PR Summary

This pr resolves #23171

The vLines and hlines functions are using incorrect coordinates.

The cause of the issue seems to be 2-fold:

  1. The transform for the axes is not passed down into the Line Collection and therefore it was not transforming the coordinates for the line placement properly.
  2. The scaling for rectilinear axes was incorrect because the data limit values are not being translated into the data coordinate system.

PR Checklist

Documentation and Tests

  • Has pytest style unit tests (and pytest passes)
  • Documentation is sphinx and numpydoc compliant (the docs should build without error).
  • New plotting related features are documented with examples.

Release Notes

  • New features are marked with a .. versionadded:: directive in the docstring and documented in doc/users/next_whats_new/
  • API changes are marked with a .. versionchanged:: directive in the docstring and documented in doc/api/next_api_changes/
  • Release notes conform with instructions in next_whats_new/README.rst or next_api_changes/README.rst

@QuLogic
Copy link
Member

QuLogic commented Feb 25, 2023

Since this is the second one with it, I don't know what this RR prefix is that you're using on your PR titles.

@robrighter
Copy link
Contributor Author

Since this is the second one with it, I don't know what this RR prefix is that you're using on your PR titles.

Sorry. It's my initials. This is a force of habit thing for me because I'm used to the convention of prefixing branch titles and consequently PRs this way. I will remove the prefix(s) and try to remember not to name PRs this way for this project.

@robrighter robrighter changed the title RR - fix axes vlines and hlines using wrong coordinates Fix axes vlines and hlines using wrong coordinates Feb 25, 2023
@robrighter robrighter force-pushed the fix-axes-vlines-hlines-wrong-coordinates branch from 80089cd to 29e4ea1 Compare March 1, 2023 05:12
@QuLogic
Copy link
Member

QuLogic commented Mar 2, 2023

Ah, I see. Please remove the prefix from the commit message as well then, as there's an Author: field for that.

@robrighter robrighter force-pushed the fix-axes-vlines-hlines-wrong-coordinates branch 2 times, most recently from 930c35b to e480841 Compare March 3, 2023 04:05
@robrighter
Copy link
Contributor Author

@QuLogic I think I have a solution here that will work to fix this issue. I have it implemented for vlines. I'd be interested to get your feedback on it. If you agree with the approach, I will go ahead and generalize it for hlines.

@robrighter robrighter force-pushed the fix-axes-vlines-hlines-wrong-coordinates branch 2 times, most recently from 8980b9c to 7fa10bf Compare March 5, 2023 01:45
@robrighter robrighter marked this pull request as ready for review March 5, 2023 01:47
oscargus
oscargus approved these changes Mar 5, 2023
View reviewed changes
Copy link
Member

@oscargus oscargus left a comment

Choose a reason for hiding this comment

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

Looks good!

(I edited the PR message to remove "issue" as "resolves #..." will automatically link this PR with the issue, so upon merging the issue is closed.)

(Seems to be quite a bit of common code in vlines and hlines, but that was the same earlier.)

QuLogic
QuLogic reviewed Mar 7, 2023
View reviewed changes
@robrighter robrighter force-pushed the fix-axes-vlines-hlines-wrong-coordinates branch 2 times, most recently from 0541220 to 8397419 Compare March 15, 2023 02:52
@jklymak jklymak requested a review from QuLogic March 28, 2023 20:43
QuLogic
QuLogic reviewed Mar 30, 2023
View reviewed changes
@QuLogic QuLogic mentioned this pull request Apr 4, 2023
Closed
@robrighter robrighter force-pushed the fix-axes-vlines-hlines-wrong-coordinates branch 2 times, most recently from e60dce3 to 217fc0d Compare April 19, 2023 02:45
@robrighter robrighter force-pushed the fix-axes-vlines-hlines-wrong-coordinates branch from 217fc0d to 33c7c2d Compare April 19, 2023 03:06
@robrighter
Copy link
Contributor Author

@QuLogic I believe all of your comments have been addressed now in the PR. I also rebased the commit.

@QuLogic
Copy link
Member

QuLogic commented Apr 19, 2023

LGTM, thanks for coming back to this.

@QuLogic QuLogic merged commit cc72116 into matplotlib:main Apr 19, 2023
@QuLogic QuLogic mentioned this pull request Apr 21, 2023
Merged
1 task
@ksunden ksunden added this to the v3.8.0 milestone Apr 25, 2023
@ksunden ksunden mentioned this pull request Jun 28, 2023
Closed
@anntzer anntzer mentioned this pull request Sep 16, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@QuLogic QuLogic QuLogic approved these changes

@oscargus oscargus oscargus approved these changes

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

Successfully merging this pull request may close these issues.

[Bug]: axes vlines() / hlines() incorrectly use data coordinate as min when blended transform is applied
4 participants
@robrighter @QuLogic @oscargus @ksunden