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

Allow non-default scales on polar axes#24825

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
oscargus merged 7 commits intomatplotlib:mainfromdstansby:polar-scales
Jan 12, 2023

Conversation

dstansby
Copy link
Member

@dstansbydstansby commentedDec 27, 2022•
edited
Loading

PR Summary

After a long excursion down the rabbit hole of polar Axes transforms, I have emerged to working log-polar axes! Unsurprisingly the fix was trivial - see82d5c5f. While I was at it the other commits add some more commenting and tidy up some code. Please leave critical reviews and suggestions, would be good to address them while I still have all this context in my head!

polar_log

Based on top of#24763

Fixes#24383
Fixes#12803
Fixes probaby#7751
Fixes probably#5527

PR Checklist

Documentation and Tests

  • Has pytest style unit tests (andpytest passes)
  • Documentation is sphinx and numpydoc compliant (the docs shouldbuild 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 indoc/users/next_whats_new/
  • API changes are marked with a.. versionchanged:: directive in the docstring and documented indoc/api/next_api_changes/
  • Release notes conform with instructions innext_whats_new/README.rst ornext_api_changes/README.rst

t *= self._axis.get_theta_direction()
t += self._axis.get_theta_offset()
theta *= self._axis.get_theta_direction()
theta += self._axis.get_theta_offset()
Copy link
Member

Choose a reason for hiding this comment

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

Is it possible to add a test that uses these lines?

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

It looks like_apply_theta_transforms=True case is not used internally, but as a backwards compatability wrapper for users who usedPolarTransform before its behaviour was changed. I think adding a meaninful test would be tricky here.

oscargus reacted with thumbs up emoji
@oscargus
Copy link
Member

oscargus commentedDec 28, 2022•
edited
Loading

Also, can you please add a test for#12803? Or is it "obvious" that this is tested by the existing one?

Oh, and possibly the other ones as well. (It seems like "Probably fixes" is interpreted as "fixes", so those will be closed upon merging this with the current statement.)

In general, I think this all looks good, but since I do not really know the area well enough I can only comment on details. (The previous PR was small enough for me to understand...)

@dstansby
Copy link
MemberAuthor

Also, can you please add a test for#12803? Or is it "obvious" that this is tested by the existing one?

I think it's obvious, in that it's the same bug that was causing both#12803#24383 (that I've fixed and added a test to).

Oh, and possibly the other ones as well. (It seems like "Probably fixes" is interpreted as "fixes", so those will be closed upon merging this with the current statement.)

I've updated the comment so these aren't closed if/when this PR is merged. After a bit more investigation I think this fixes them a bit, but not fully.

Copy link
Member

@oscargusoscargus 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 as far as I can tell!

Unsure about 3.7 or 3.8, so I'll try to get some clarity during the dev-call tonight and then merge accordingly.

ax = fig.add_subplot(polar=True)

ax.set_yscale('log')
ax.set_ylim(1, 1000)
Copy link
Member

Choose a reason for hiding this comment

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

For clarity, I would useset_rscale andset_rlim.

@tacaswelltacaswell added this to thev3.7.0 milestoneJan 12, 2023
Co-authored-by: Thomas A Caswell <tcaswell@gmail.com>
@oscargusoscargus merged commitba03951 intomatplotlib:mainJan 12, 2023
meeseeksmachine pushed a commit to meeseeksmachine/matplotlib that referenced this pull requestJan 12, 2023
@dstansbydstansby deleted the polar-scales branchJanuary 12, 2023 21:19
oscargus added a commit that referenced this pull requestJan 12, 2023
…825-on-v3.7.xBackport PR#24825 on branch v3.7.x (Allow non-default scales on polar axes)
@ksundenksunden mentioned this pull requestFeb 20, 2023
6 tasks
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@efiringefiringefiring left review comments

@tacaswelltacaswelltacaswell left review comments

@ksundenksundenksunden approved these changes

@oscargusoscargusoscargus approved these changes

Assignees
No one assigned
Projects
None yet
Milestone
v3.7.0
Development

Successfully merging this pull request may close these issues.

log scale and polar broken pcolormesh in log polar coordinates
5 participants
@dstansby@oscargus@efiring@tacaswell@ksunden
[8]ページ先頭
©2009-2025 Movatter.jp