Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

Ensure polar plot radial lower limit remains at 0 after set_rticks + plot#29798

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
rcomer merged 9 commits intomatplotlib:mainfrombruswaine:bugfix-for-issue-29528
Mar 29, 2025

Conversation

bruswaine
Copy link
Contributor

@bruswainebruswaine commentedMar 24, 2025•
edited
Loading

Update: The final implementation differs from the original proposal. The solution now preserves theRadialLocator behavior that was being overwritten onset_rticks instead of modifyinghandle_single_axis. The test was renamed totest_radial_limits_behavior.

PR summary

  • Why is this change necessary?
    When usingset_rticks followed byplot 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 withplot.

  • What is the reasoning for this implementation?

    • Modifiedhandle_single_axis inautoscale_view to explicitly enforce lower limit of 0 for polar plots
    • Added testtest_radial_limit_after_plot to verify limit stays at 0 afterset_rticks +plot

Example:

fig = plt.figure()ax = fig.add_subplot(projection='polar')ax.set_rticks([1, 2, 3, 4])  # Would previously allow lower limit > 0ax.plot([0, 1], [2, 3])      # Now maintains lower limit at 0

Closes#29528

PR checklist

…from zeroAfter calling `set_rticks`, the lower radial limit was beingoverwritten when `plot` was called. This was due to `autoscale_view`logic not enforcing the constraint that the lower radial limit must be0 in polar plots. This fix ensures that the lower radial limit isalways 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 thatthe lower limit of the radial axis is 0.
Copy link

@github-actionsgithub-actionsbot left a comment

Choose a reason for hiding this comment

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

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 uson gitter for real-time discussion.

For details on testing, writing docs, and our review process, please seethe developer guide

We strive to be a welcoming and open project. Please follow ourCode of Conduct.

@rcomer
Copy link
Member

rcomer commentedMar 24, 2025•
edited
Loading

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.

@bruswaine
Copy link
ContributorAuthor

Thanks@rcomer for the quick reply.

I've seen your comment on the Issue where you show theview_limits function.
Would adding more checks for those cases suffice for the fix? For instancex0 = min(0, x0) instead of justx0 = 0.
I've also noticed that some functions call the super and then set aRadialLocator.That would that be a cleaner solution, as it would keep all the logic already set for polar plots. I will investigate this further and test it out.

…from 0.After calling `set_rticks`, the lower radial limit was beingoverwritten when `plot` was called. This was due to `set_rticks`overwriting the `RadialLocator` wrapper for a `FixedLocator` losingall 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 thatthe lower limit of the radial axis is maintained after `set_rticks`.
…from 0.After calling `set_rticks`, the lower radial limit was beingoverwritten when `plot` was called. This was due to `set_rticks`overwriting the `RadialLocator` wrapper for a `FixedLocator` losingall 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 thatthe lower limit of the radial axis is maintained after `set_rticks`.
Co-authored-by: Tim Hoffmann <2836374+timhoffm@users.noreply.github.com>
@bruswaine
Copy link
ContributorAuthor

bruswaine commentedMar 25, 2025•
edited
Loading

@timhoffm Note this PR also incidentally fixes#29516 (Cannot draw arrow from polar plot origin after setting rticks
As it's related to the origin being away from zero afterset_rticks i didn't include any test for this specific issue but let me know if it's needed, i'd be happy to.

@rcomer
Copy link
Member

rcomer commentedMar 25, 2025•
edited
Loading

@timhoffm Note this PR also incidentally fixes#29516 (Cannot draw arrow from polar plot origin after setting rticks As it's related to the origin being away from zero afterset_rticks i didn't include any test for this specific issue but let me know if it's needed, i'd be happy to.

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

@bruswaine
Copy link
ContributorAuthor

@timhoffm Thanks for the clarification.
Sorry for my lack of knowledge as this is my first contribution to an open source project, is there anything else i should do after i accepted your suggested changes?

Copy link
Member

@rcomerrcomer left a comment

Choose a reason for hiding this comment

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

I’m pretty sure the Appveyor failure is unrelated.

@rcomerrcomer added topic: ticks axis labels PR: bugfixPull requests that fix identified bugs labelsMar 26, 2025
Comment on lines +1296 to +1297
self.yaxis.set_major_locator(
self.RadialLocator(self.yaxis.get_major_locator(), self))
Copy link
Member

Choose a reason for hiding this comment

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

While this solves the particuar issue, I'm unclear whether this is the right fix:

  • Is this the right place to fix this? Or would it be better to overrideRadialAxis.set_major_locator to always wrap in a RadialLocator?
  • Not an expert here, but why do we manually warp in a RadialLocator and not use_warp_locator_formatter()? The difference is that the latter setsself.isDefault_majloc = True, which apparently has to do with unit converters, but the purpose and correct handling is quite unclear to me.

Copy link
Member

Choose a reason for hiding this comment

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

I think that if someone callsset_major_locator directly then we should respect it and used what they passed. TheRadialLocator 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 wantRadialLocator then I don't think they have a way around the auto-wrapping? Here's a slightly esoteric example:

importmatplotlib.pyplotaspltimportmatplotlib.datesasmdatesfrommatplotlib.projections.polarimportRadialLocatorimportnumpyasnpdates=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()

image
image
image

Ithink settingself.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 callingplot and/oryaxis_date, then the logic that usesisDefault_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')

image

Co-authored-by: Tim Hoffmann <2836374+timhoffm@users.noreply.github.com>
@rcomerrcomer added this to thev3.11.0 milestoneMar 29, 2025
Co-authored-by: Ruth Comer <10599679+rcomer@users.noreply.github.com>
@rcomerrcomer merged commit2933275 intomatplotlib:mainMar 29, 2025
39 of 41 checks passed
@github-project-automationgithub-project-automationbot moved this fromNeeds review toWaiting for author inFirst Time ContributorsMar 29, 2025
@rcomer
Copy link
Member

Thanks@bruswaine and congratulations on your first open source PR 🎉 Hopefully we will hear from you again.

@bruswaine
Copy link
ContributorAuthor

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.

timhoffm reacted with thumbs up emojircomer reacted with heart emoji

@QuLogicQuLogic moved this fromWaiting for author toMerged inFirst Time ContributorsMar 30, 2025
timhoffm added a commit to timhoffm/matplotlib that referenced this pull requestJun 13, 2025
All the wrapping logic is now contained in RadialAxisClosesmatplotlib#30164 and rearchitectsmatplotlib#29798.
timhoffm added a commit to timhoffm/matplotlib that referenced this pull requestJun 16, 2025
All the wrapping logic is now contained in RadialAxisClosesmatplotlib#30164 and rearchitectsmatplotlib#29798.
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@rcomerrcomerrcomer approved these changes

@github-actionsgithub-actions[bot]github-actions[bot] left review comments

@timhoffmtimhoffmtimhoffm approved these changes

Assignees
No one assigned
Labels
PR: bugfixPull requests that fix identified bugstopic: polar
Projects
Milestone
v3.11.0
Development

Successfully merging this pull request may close these issues.

[Bug]: set_rticks makes polar autoscale move the origin away from zero
3 participants
@bruswaine@rcomer@timhoffm
[8]ページ先頭
©2009-2025 Movatter.jp