-
-
Notifications
You must be signed in to change notification settings - Fork 7.8k
Polar limits enhancements #4699
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
Conversation
The improvements you list are long overdue. Thanks very much for taking this on. I hope you will also be able to fix the zooming and panning, so that it works in display space rather than in data space, but preserves the aspect ratio. In other words, it should be like using a magnifying glass to look at a map. |
Where is zooming implemented? Obviously, it's not as simple as just setting |
It's confusing because there are two kinds of zoom: the zoom box (zoom-to-rect), and the continuous zoom/pan. The latter is implemented in the PolarAxes.drag_pan() method, but for normal axes drag_pan and all the other zoom-to-rect and pan/zoom methods are in the backend_bases.py NavigationToolbar2. There are PRs in the works to refactor some of these things. @OceanWolf, can you provide some advice on how to proceed? Now I'm thinking that maybe at least for the zoom-to-rect functionality, @QuLogic should defer action until the refactoring is merged. |
Ah, looking at that code, it makes sense that polar axes don't zoom quite right (if I enable it). Instead of applying some sort of cropped zoom, it just modifies the x/y limits, which in polar axes does not exactly produce a box, and of course wouldn't work before when you couldn't modify the limits at all... Though once I get the plot to actually crop to the wedge instead of leaving the full circle, maybe that won't be so bad. |
Don't have time to look at this right now, so very rough advice. New toolbar has been delayed until next release due to review delays, but as I don't think this will go in either for then, not a problem. Basically in terms of zoom/panning, just take a look at the code in master in |
@OceanWolf: FYI, that does not work: File ".../lib/matplotlib/pyplot.py", line 518, in figure
**kwargs)
File ".../lib/matplotlib/backends/backend_tkagg.py", line 84, in new_figure_manager
return new_figure_manager_given_figure(num, figure)
File ".../lib/matplotlib/backends/backend_tkagg.py", line 112, in new_figure_manager_given_figure
figManager = FigureManagerTkAgg(canvas, num, window)
File ".../lib/matplotlib/backends/backend_tkagg.py", line 545, in __init__
backend_tools.add_tools_to_manager(self.toolmanager)
File ".../lib/matplotlib/backend_tools.py", line 971, in add_tools_to_manager
toolmanager.add_tool(name, tool)
File ".../lib/matplotlib/backend_managers.py", line 221, in add_tool
tool_cls = self._get_cls_to_instantiate(tool)
File ".../lib/matplotlib/backend_managers.py", line 307, in _get_cls_to_instantiate
globals(), locals(), [mod], -1)
ValueError: level must be >= 0 Probably a Python 3 issue. |
So this is nearly complete (WRT original goals) except for a couple issues:
I will try and tackle the pan/zoom and add some change documentation as well, but it would be helpful to get some insight on the above first. |
lib/matplotlib/projections/polar.py
Outdated
# Scale view limit into a bbox around the selected wedge. This may be | ||
# smaller than the usual unit axes rectangle if not plotting the full | ||
# circle. | ||
self.axesLim = _WedgeBbox((0.5, 0.5), self._realViewLim) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if axesLim
is required to be unitized, but it isn't here, necessarily (on purpose, of course.)
7b4fb1f
to
6cf4b39
Compare
I added some "what's new" text. @efiring I can probably implement the modified zoom/pan, but the API between backend and axis is not very nice. The trouble is that the backend seems to think that the only relevant information is the view limit. That makes no sense on a polar axis if you want to use cartesian zoom. If I implement pan/zoom using an additional transform (for example), then the home/back/forth buttons will not work. |
Maybe the thing to do is skip the Cartesian zoom/pan for the present PR, and enter it as a new wish-list issue. Then, if you are willing, you could attack it separately, making whatever larger changes are needed so that everything could work--presumably for other coordinate systems in addition to polar. @pelson, how have you handled zoom/pan in cartopy? Any ideas about this? |
I am a bit concerned by the number of test images that have been changed. |
Because the |
BTW, I should probably note that those image changes were ignored by the default threshold. The only reason I noticed is because one test image was wrong, and the difference was ignored until the added 0.0 triggered a failure. |
Those zero-ticks don't look good; maybe they need to be special-cased away. I would consider them a regression. |
I was debating that, but what should happen when you add an offset? Right now, you get a tick, but previously, you wouldn't. |
IIUC, Cartopy "cheats" by always plotting the projected space, which is in Cartesian coordinates. |
On 2015/07/22 9:07 AM, Elliott Sales de Andrade wrote:
That's what Basemap does, but I thought Cartopy was using the mpl |
@mdboom, do you have any thoughts about this? The problem is that for non-affine projections like polar, we would like zoom and pan to work in Cartesian space. Zooming radially inward in a polar plot is rarely useful, if ever. |
I should note that this issue is sort of similar to complaints about On Wed, Jul 22, 2015 at 3:27 PM, Eric Firing notifications@github.com
|
@efiring OK, the central tick is now not shown if you are plotting a full non-annular circle. In the other cases, I feel there is no benefit to hiding it. It looks like there are enough minor changes for GitHub to still think most of the test images changed, but if you look at the actual diff, they haven't really. I will probably have to squash the change to make it happy (and I will anyway to avoid the extraneous binary diffs.) |
I don't really understand why this is not passing on CI; it works fine locally and I made sure to use the correct local FreeType setting. |
e28eae2
to
e9be315
Compare
This can be used in the polar axes when plotting less than the full circle.
Polar axes do not have any issue with negative radii; the plotting axes are simply shifted so that the minimum radius of the data occurs at r=0 in plotting space. For all-positive radii, the RadialLocator will preferentially choose 0 as the minimum view limit to remain backwards compatible with the previous version. If any radii are negative, then the usual minimum is used.
This is related to a warning seen in matplotlib#2827, but doesn't fix the main bug.
There are so many imports from transforms that it's getting a bit unwieldy. Simpler to just import as mtransforms like elsewhere.
This enables some nicer automatic invalidation for later work.
This means that the minimum radius can be offset to not occur right at the middle of the plot.
Setting angular limits allows one to create "wedges" instead of a full circle or annulus.
Let the user specify an arbitrary zero location with reference to the usual cardinal points. This can be a bit more intuitive than requiring the offset in radians.
This ensures they're rotated the same if plotting full circle and using the rlabel position mode.
It's not generally necessary on Cartesian plots, but can be troublesome with wedge-like polar plots where the spines can overlap at acute angles.
Also, switch to default style for all updated images.
It took entirely too long to figure out that when |
@QuLogic Thank you for sticking with this. These are wonderful improvements. |
Thanks! |
This is a
WIPPR to add several enhancements to polar plots.