FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Allow non-default scales on polar axes by dstansby · Pull Request #24825 · matplotlib/matplotlib · GitHub

  1. matplotlib
  2. matplotlib

Repository navigation

Allow non-default scales on polar axes - #24825

Merged
oscargus merged 7 commits into
matplotlib:mainfrom
dstansby:polar-scales
Jan 12, 2023
ConversationCommits (7)ChecksFiles changed
Merged

Allow non-default scales on polar axes#24825
oscargus merged 7 commits into
matplotlib:mainfrom
dstansby:polar-scales

Conversation

dstansby commented Dec 27, 2022 •
edited
Loading

Copy link
Copy Markdown
Member

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 - see 82d5c5f. 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!

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 (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

oscargus reviewed Dec 28, 2022
View reviewed changes
Comment thread lib/matplotlib/projections/polar.py Outdated
oscargus reviewed Dec 28, 2022
View reviewed changes
Comment thread lib/matplotlib/projections/polar.py
t *= self._axis.get_theta_direction()
t += self._axis.get_theta_offset()
theta *= self._axis.get_theta_direction()
theta += self._axis.get_theta_offset()

oscargus Dec 28, 2022

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

dstansby Dec 28, 2022

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

oscargus commented Dec 28, 2022 •
edited
Loading

Copy link
Copy Markdown
Member

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 commented Dec 28, 2022

Copy link
Copy Markdown
Member Author

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.

anntzer mentioned this pull request Dec 29, 2022
Closed
dstansby marked this pull request as draft December 31, 2022 11:09
dstansby marked this pull request as ready for review January 1, 2023 17:45
dstansby requested a review from oscargus January 10, 2023 15:38
oscargus modified the milestone: v3.7.0 Jan 12, 2023
oscargus approved these changes Jan 12, 2023
View reviewed changes

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

efiring reviewed Jan 12, 2023
View reviewed changes
Comment thread lib/matplotlib/tests/test_polar.py Outdated
ax = fig.add_subplot(polar=True)

ax.set_yscale('log')
ax.set_ylim(1, 1000)

efiring Jan 12, 2023

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

For clarity, I would use set_rscale and set_rlim.

tacaswell added this to the v3.7.0 milestone Jan 12, 2023
tacaswell reviewed Jan 12, 2023
View reviewed changes
Comment thread lib/matplotlib/tests/test_polar.py Outdated
Rename y -> r
a24daae 
Co-authored-by: Thomas A Caswell <tcaswell@gmail.com>
oscargus merged commit ba03951 into matplotlib:main Jan 12, 2023
meeseeksmachine mentioned this pull request Jan 12, 2023
Merged
meeseeksmachine pushed a commit to meeseeksmachine/matplotlib that referenced this pull request Jan 12, 2023
Backport PR matplotlib#24825: Allow non-default scales on polar axes
9b30fdf
dstansby deleted the polar-scales branch January 12, 2023 21:19
oscargus added a commit that referenced this pull request Jan 12, 2023
Merge pull request #24960 from meeseeksmachine/auto-backport-of-pr-24…
81ba63f 
…825-on-v3.7.x

Backport PR #24825 on branch v3.7.x (Allow non-default scales on polar axes)
ksunden mentioned this pull request Feb 20, 2023
Merged
6 tasks
dhomeier mentioned this pull request Feb 28, 2023
Merged
tacaswell mentioned this pull request Jan 3, 2024
Closed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

efiring efiring left review comments
tacaswell tacaswell left review comments
ksunden ksunden approved these changes
oscargus oscargus approved these changes

Assignees

No one assigned

Labels

topic: polar

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

Back | FazBrowse Home | New Git URL