| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
Sorry, something went wrong.
…from zero After calling `set_rticks`, the lower radial limit was being overwritten when `plot` was called. This was due to `autoscale_view` logic not enforcing the constraint that the lower radial limit must be 0 in polar plots. This fix ensures that the lower radial limit is always enforced by explicitly setting it to 0 inside `handle_single_axis` (inside `autoscale_view`). Added `test_radial_limit_after_plot` to `test_polar.py` to assert that the lower limit of the radial axis is 0.
There was a problem hiding this comment.
Thank you for opening your first PR into Matplotlib!
If you have not heard from us in a week or so, please leave a new comment below and that should bring it to our attention. Most of our reviewers are volunteers and sometimes things fall through the cracks.
You can also join us on gitter for real-time discussion.
For details on testing, writing docs, and our review process, please see the developer guide
We strive to be a welcoming and open project. Please follow our Code of Conduct.
Sorry, something went wrong.
|
Thank you @bruswaine for your work on this. However, this is not quite the right fix. As shown by the failing tests, we do expect the radial lower limit to sometimes be negative, and sometimes r is on a log-scale. |
Sorry, something went wrong.
|
Thanks @rcomer for the quick reply. I've seen your comment on the Issue where you show the view_limits function. |
Sorry, something went wrong.
…from 0. After calling `set_rticks`, the lower radial limit was being overwritten when `plot` was called. This was due to `set_rticks` overwriting the `RadialLocator` wrapper for a `FixedLocator` losing all Radial logic. This fix sets the RadialLocator inside `set_rticks` after calling `Axes.set_yticks` ensuring all radial logic is applied. Added `test_radial_limits_behavior` to `test_polar.py` to assert that the lower limit of the radial axis is maintained after `set_rticks`.
…from 0. After calling `set_rticks`, the lower radial limit was being overwritten when `plot` was called. This was due to `set_rticks` overwriting the `RadialLocator` wrapper for a `FixedLocator` losing all Radial logic. This fix sets the RadialLocator inside `set_rticks` after calling `Axes.set_yticks` ensuring all radial logic is applied. Added `test_radial_limits_behavior` to `test_polar.py` to assert that the lower limit of the radial axis is maintained after `set_rticks`.
Co-authored-by: Tim Hoffmann <2836374+timhoffm@users.noreply.github.com>
|
@timhoffm Note this PR also incidentally fixes #29516 (Cannot draw arrow from polar plot origin after setting rticks |
Sorry, something went wrong.
That issue was caused by a combination of two problems, and you fixed one of them. To close that issue, we should also fix the examples given at #29516 (comment). |
Sorry, something went wrong.
|
@timhoffm Thanks for the clarification. |
Sorry, something went wrong.
There was a problem hiding this comment.
I’m pretty sure the Appveyor failure is unrelated.
Sorry, something went wrong.
| self.yaxis.set_major_locator( | ||
| self.RadialLocator(self.yaxis.get_major_locator(), self)) |
There was a problem hiding this comment.
While this solves the particuar issue, I'm unclear whether this is the right fix:
Sorry, something went wrong.
There was a problem hiding this comment.
I think that if someone calls set_major_locator directly then we should respect it and used what they passed. The RadialLocator is public so, if that is what they really need, they can wrap themselves before passing it. If we always wrap and for some reason they didn't want RadialLocator then I don't think they have a way around the auto-wrapping? Here's a slightly esoteric example:
import matplotlib.pyplot as plt
import matplotlib.dates as mdates
from matplotlib.projections.polar import RadialLocator
import numpy as np
dates = np.arange('2025-01-01', '2027-01-01', dtype='datetime64[D]')
theta = np.linspace(0, np.pi * 4, 365 * 2)
fig, ax = plt.subplots(subplot_kw=dict(projection='polar'))
ax.plot(theta, dates)
ax.set_title('Default Ticks')
fig, ax = plt.subplots(subplot_kw=dict(projection='polar'))
ax.yaxis.set_major_locator(mdates.YearLocator())
ax.plot(theta, dates)
ax.set_title('Year Locator')
fig, ax = plt.subplots(subplot_kw=dict(projection='polar'))
ax.yaxis.set_major_locator(RadialLocator(mdates.YearLocator(), ax))
ax.plot(theta, dates)
ax.set_title('Year Locator wrapped with RadialLocator')
plt.show()I think setting self.isDefault_majloc = True allows the locator to be replaced with one defined by the converter. If we have explicitly set the ticks, then I think we don't want that. I tried to demonstrate something about this with my date example, but if I set the ticks before calling plot and/or yaxis_date, then the logic that uses isDefault_majloc never got called and I don't have date formatting 😕
fig, ax = plt.subplots(subplot_kw=dict(projection='polar'))
ax.set_rticks(dates[::200])
ax.yaxis_date()
ax.plot(theta, dates)
ax.set_title('Set ticks')
Sorry, something went wrong.
Co-authored-by: Tim Hoffmann <2836374+timhoffm@users.noreply.github.com>
Co-authored-by: Ruth Comer <10599679+rcomer@users.noreply.github.com>
|
Thanks @bruswaine and congratulations on your first open source PR 🎉 Hopefully we will hear from you again. |
Sorry, something went wrong.
|
Thank you @rcomer and @timhoffm for your guidance throughout this PR! I really appreciated your clear feedback and patience. This was a great learning experience, and I'm looking forward to contributing more to Matplotlib in the future. |
Sorry, something went wrong.
All the wrapping logic is now contained in RadialAxis Closes matplotlib#30164 and rearchitects matplotlib#29798.
All the wrapping logic is now contained in RadialAxis Closes matplotlib#30164 and rearchitects matplotlib#29798.
All the wrapping logic is now contained in RadialAxis Closes matplotlib#30164 and rearchitects matplotlib#29798.
| Back | FazBrowse Home | New Git URL |
Update: The final implementation differs from the original proposal. The solution now preserves the RadialLocator behavior that was being overwritten on set_rticks instead of modifying handle_single_axis. The test was renamed to test_radial_limits_behavior.
PR summary
Why is this change necessary?
When using set_rticks followed by plot in polar plots, the lower radial limit (which should
always be 0) was being overwritten by autoscaling logic. This caused visual artifacts and
incorrect axis behavior in polar plots.
What problem does it solve?
This fix prevents the radial origin from moving away from 0 when setting radial ticks with
set_rticks and adding data with plot.
What is the reasoning for this implementation?
Example:
Closes #29528
PR checklist