Skip to content

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

Merged
merged 14 commits into from
Aug 21, 2017
Merged

Polar limits enhancements #4699

merged 14 commits into from
Aug 21, 2017

Conversation

QuLogic
Copy link
Member

@QuLogic QuLogic commented Jul 15, 2015

This is a WIP PR to add several enhancements to polar plots.

@tacaswell tacaswell added this to the proposed next point release milestone Jul 15, 2015
@efiring
Copy link
Member

efiring commented Jul 15, 2015

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.

@QuLogic
Copy link
Member Author

QuLogic commented Jul 17, 2015

Where is zooming implemented? Obviously, it's not as simple as just setting can_zoom to return True. Is it just a special case in start/end_pan?

@efiring
Copy link
Member

efiring commented Jul 17, 2015

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.

@QuLogic
Copy link
Member Author

QuLogic commented Jul 17, 2015

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.

@QuLogic
Copy link
Member Author

QuLogic commented Jul 17, 2015

Well, I'm finally getting there...
output
You can use the zoom button to zoom in, though it produces wedges instead of a rectangular crop. The pan/zoom button hasn't been changed yet.

There are a few hacks though, which I'll have to figure out eventually. Right now, in order to update the background patch, I've had to make the PolarAxes pretend it's a parent to the view/radial limits. Maybe there's a simpler way around that. Also, you'll notice that the background patch does not seem to be clipping the data. Is there some kind of update that I need to call?

Theta text 1 and text 2 are swapped now, so the tests are going to break. I'm not sure if I can switch them back or whether their visibility should just be swapped.

The transforms were hacked on until they worked, so I'm pretty sure they can be optimized slightly.

@OceanWolf
Copy link
Member

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 backend_tools.py, that contains everything you need to know about the new toolbar. You can test it out on Gtk3 and/or TkAgg by setting rcParam['toolbar'] = 'toolmanager'

@QuLogic
Copy link
Member Author

QuLogic commented Jul 19, 2015

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

@QuLogic
Copy link
Member Author

QuLogic commented Jul 21, 2015

So this is nearly complete (WRT original goals) except for a couple issues:

  • When I update the axes patch, none of the data lines/points/etc. are clipped! How do I get the other patches to re-clip themselves?
  • The use of _invalidate_internal is a bit of a hack. It takes care of updating the axes patch dimensions, and the visibility of spines whenever any of the view parameters are changed. I was uncertain as to how the viewLim might be altered, so I found this to be the safest method. But if there is some sort of guarantee that it's only modified by method calls, I could make this more explicit.
  • No matter what I return for the horizontal or vertical alignments of the radial tick labels on the first axis, they always seem to be in the same place! The second axis appears to work nicely.

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.

# 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)
Copy link
Member Author

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

@QuLogic QuLogic force-pushed the polar-limits branch 3 times, most recently from 7b4fb1f to 6cf4b39 Compare July 22, 2015 07:53
@QuLogic
Copy link
Member Author

QuLogic commented Jul 22, 2015

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.

@efiring
Copy link
Member

efiring commented Jul 22, 2015

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?

@tacaswell
Copy link
Member

I am a bit concerned by the number of test images that have been changed.

@QuLogic
Copy link
Member Author

QuLogic commented Jul 22, 2015

Because the RadialLocator no longer limits ticks to positive values, the test results now include a tick at 0.0.

@QuLogic
Copy link
Member Author

QuLogic commented Jul 22, 2015

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.

@efiring
Copy link
Member

efiring commented Jul 22, 2015

Those zero-ticks don't look good; maybe they need to be special-cased away. I would consider them a regression.

@QuLogic
Copy link
Member Author

QuLogic commented Jul 22, 2015

I was debating that, but what should happen when you add an offset? Right now, you get a tick, but previously, you wouldn't.

@QuLogic
Copy link
Member Author

QuLogic commented Jul 22, 2015

@pelson, how have you handled zoom/pan in cartopy? Any ideas about this?

IIUC, Cartopy "cheats" by always plotting the projected space, which is in Cartesian coordinates.

@efiring
Copy link
Member

efiring commented Jul 22, 2015

On 2015/07/22 9:07 AM, Elliott Sales de Andrade wrote:

IIUC, Cartopy "cheats" by always plotting the projected space, which is
in Cartesian coordinates.

That's what Basemap does, but I thought Cartopy was using the mpl
transform machinery. Maybe not.

@efiring
Copy link
Member

efiring commented Jul 22, 2015

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

@WeatherGod
Copy link
Member

I should note that this issue is sort of similar to complaints about
mplot3d's "zooming" and "panning".

On Wed, Jul 22, 2015 at 3:27 PM, Eric Firing notifications@github.com
wrote:

@mdboom https://github.com/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.


Reply to this email directly or view it on GitHub
#4699 (comment)
.

@QuLogic
Copy link
Member Author

QuLogic commented Jul 22, 2015

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

@QuLogic
Copy link
Member Author

QuLogic commented Aug 11, 2017

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.

@QuLogic QuLogic force-pushed the polar-limits branch 2 times, most recently from e28eae2 to e9be315 Compare August 15, 2017 10:06
QuLogic added 14 commits August 15, 2017 22:19
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.
@QuLogic
Copy link
Member Author

QuLogic commented Aug 16, 2017

It took entirely too long to figure out that when reset_ticks is called, I also needed to re-add the clip path on the ticks. Anyway, this seems to be back to working again.

@efiring efiring merged commit 06e389f into matplotlib:master Aug 21, 2017
@efiring
Copy link
Member

efiring commented Aug 21, 2017

@QuLogic Thank you for sticking with this. These are wonderful improvements.

@QuLogic QuLogic deleted the polar-limits branch August 21, 2017 20:04
@QuLogic
Copy link
Member Author

QuLogic commented Aug 21, 2017

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Release critical For bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions. topic: polar
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants