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

Fix axh{line,span} on polar axes. #26788

Merged
merged 1 commit into from
Sep 27, 2023
Merged

Fix axh{line,span} on polar axes. #26788

merged 1 commit into from
Sep 27, 2023

Conversation

anntzer
Copy link
Contributor

@anntzer anntzer commented Sep 15, 2023

For axhline, set the underlying path's _interpolation_steps to GRIDLINE_INTERPOLATION_STEPS (180), which amounts to using the same logic to draw it as for drawing gridlines. This ensures that a polar axhline goes (due to interpolation) around the whole circle, rather than being a trivial line connecting a point to itself. Also update axvline for consistency. (Note that _interpolation_steps has is ignored for rectilinear transforms and thus the change doesn't slow down the common, rectilinear case.)

Increase the number of interpolation steps of ax{v,h}span likewise to GRIDLINE_INTERPOLATION_STEPS (again for consistency), and more importantly switch them to using Rectangle rather than Polygon, so that a polar axhspan is drawn as an annulus. This is necessary due to a separate "bug", whereby the CLOSEPOLY step on a Polygon would generate (effectively) a segment that's unfortunately ignored by _interpolation_steps (as it doesn't appear explicitly), whereas Rectangle explicitly includes the closing (4th) segment before emitting a CLOSEPOLY.

Closes #24996.

PR summary

PR checklist

Copy link
Contributor

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

Maybe worth a "What's new"-note?

(Subject to fixing the tests...)

@QuLogic
Copy link
Member

QuLogic commented Sep 15, 2023

Since it's not in the test, just wondering what happens when you pass [xy]{min,max} as well?

@anntzer
Copy link
Contributor Author

anntzer commented Sep 16, 2023

Tests should hopefully be fixed now, and I detailed the changes a bit more in the behavior changes note -- I can add a separate whatsnew if you think it's needed. Limiting the "other" direction in spans produces the expected wedges, now included in the test.

@oscargus
Copy link
Contributor

oscargus commented Sep 17, 2023

I think a what's new note is not required, but would be nice to highlight that this now works.

@anntzer
Copy link
Contributor Author

anntzer commented Sep 17, 2023

Added one.

Comment on lines 4 to 5
... now draw circles and circular arcs (`~.axhline`) or full or partial annuli
(`.axhspan`).
Copy link
Member

Choose a reason for hiding this comment

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

Note that the references in this file are not resolved by sphinx (it finds both the Axes method and pyplot), and are causing the doc build to fail

Suggested change
... now draw circles and circular arcs (`~.axhline`) or full or partial annuli
(`.axhspan`).
... now draw circles and circular arcs (`~.Axes.axhline`) or full or partial annuli
(`.Axes.axhspan`).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oops, fixed.

lib/matplotlib/axes/_axes.py Outdated Show resolved Hide resolved
lib/matplotlib/axes/_axes.py Outdated Show resolved Hide resolved
# For Rectangles and non-separable transforms, add_patch can be buggy
# and update the x limits even though it shouldn't do so for an
# yaxis_transformed patch, so undo that update.
Copy link
Member

Choose a reason for hiding this comment

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

There is some cutout in _update_patch_limits for x/y axis transforms; is that something that should be changed instead?

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'll look into that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The problem is that Rectangle.get_transform() is not equal to get_xaxis/yaxis_transform, because Rectangle is implemented as a unit rectangle plus a sizing and translation affine transform, which ends up being included (first!) in Rectangle.get_transform. I guess ideally we'd want to check something like (-patch.get_patch_transform() + patch.get_transform()) (the subtraction needs to be "on the left"), but the transform system can't even handle that, so for now I don't think that would be so easy.

For axhline, set the underlying path's _interpolation_steps to
GRIDLINE_INTERPOLATION_STEPS (180), which amounts to using the same
logic to draw it as for drawing gridlines.  This ensures that a polar
axhline goes (due to interpolation) around the whole circle, rather than
being a trivial line connecting a point to itself.  Also update axvline
for consistency.  (Note that _interpolation_steps has is ignored for
rectilinear transforms and thus the change doesn't slow down the common,
rectilinear case.)

Increase the number of interpolation steps of ax{v,h}span likewise to
GRIDLINE_INTERPOLATION_STEPS (again for consistency), and more
importantly switch them to using Rectangle rather than Polygon, so that
a polar axhspan is drawn as an annulus.  This is necessary due to a
separate "bug", whereby the CLOSEPOLY step on a Polygon would generate
(effectively) a segment that's unfortunately ignored by
_interpolation_steps (as it doesn't appear explicitly), whereas
Rectangle explicitly includes the closing (4th) segment before emitting
a CLOSEPOLY.
@QuLogic QuLogic added this to the v3.9.0 milestone Sep 27, 2023
@QuLogic QuLogic merged commit 870f479 into matplotlib:main Sep 27, 2023
39 of 40 checks passed
@anntzer anntzer deleted the avhlsi branch September 27, 2023 21:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: for non-rectilinear axes, axvline/axhline should behave as "draw a gridline at that x/y"
4 participants