Skip to content

Fix axes vlines and hlines using wrong coordinates - #25324

Merged
QuLogic merged 1 commit into
matplotlib:mainfrom
robrighter:fix-axes-vlines-hlines-wrong-coordinates
Apr 19, 2023
Merged

Fix axes vlines and hlines using wrong coordinates#25324
QuLogic merged 1 commit into
matplotlib:mainfrom
robrighter:fix-axes-vlines-hlines-wrong-coordinates

Conversation

@robrighter

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

Copy link
Copy Markdown
Contributor

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

QuLogic commented Feb 25, 2023

Copy link
Copy Markdown
Member

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

robrighter commented Feb 25, 2023

Copy link
Copy Markdown
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

QuLogic commented Mar 2, 2023

Copy link
Copy Markdown
Member

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

robrighter commented Mar 3, 2023

Copy link
Copy Markdown
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

@oscargus oscargus left a comment

Copy link
Copy Markdown
Member

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
Comment thread lib/matplotlib/axes/_axes.py Resolved
Comment thread lib/matplotlib/axes/_axes.py Resolved
@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
Comment thread lib/matplotlib/axes/_axes.py Resolved
Comment thread lib/matplotlib/tests/test_axes.py Resolved
@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

robrighter commented Apr 19, 2023

Copy link
Copy Markdown
Contributor Author

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

@QuLogic

QuLogic commented Apr 19, 2023

Copy link
Copy Markdown
Member

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 done
@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