Skip to content
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

ENH: Add API to manage view state in custom Axes. #4795

Merged
merged 2 commits into from Aug 27, 2015

Conversation

QuLogic
Copy link
Member

@QuLogic QuLogic commented Jul 26, 2015

Custom Axes may require additional information than simply the x/y-limits to describe the view. This API will allow the navigation buttons on the toolbar to function correctly with such Axes (but not necessarily the zoom or pan buttons just yet.)

This PR is simply a quick-to-implement idea; I have not yet applied it anywhere, though the intended usage is in PolarAxes, of course. From discussions on #4699, it seems like something like this might be useful for mplot3d as well.

There's also the question of how to update the view. As we all know by now, setting the x/y-limits does not work if the axes are not Cartesian. But NavigationToolbar2.release_zoom does quite a bit of other stuff before setting the limits and I'm not sure how to reconcile this at the moment.

default implementation saves the view limits. You *must* implement
:meth:`restore_view` if you implement this method.
"""
raise NotImplementedError
Copy link
Member

Choose a reason for hiding this comment

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

It would be simpler to put the existing code for ordinary axes here instead of leaving these as NotImplementedError, wouldn't it? I don't see any disadvantage, but I'm probably missing something.

Copy link
Member Author

Choose a reason for hiding this comment

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

Probably; I'm more curious about the API at the moment.

@efiring
Copy link
Member

efiring commented Jul 26, 2015

attn: @OceanWolf, @fariza

@fariza
Copy link
Member

fariza commented Jul 26, 2015

I proposed something similar #2511 but it was rejected.
At the end, I added the "history" as a tool backend_tools.ToolViewsPositionsin the MEP22 implementation.
I would recommend to modify this tool to hold this information.
NavigationToolbar2 is going to be deprecated soon, I wouldn't invest too much effort to add functionality to it.

@QuLogic
Copy link
Member Author

QuLogic commented Jul 26, 2015

That was a slightly different proposal, though. I am not suggesting that the Axes store their own history, just that the history tool (wherever it is) asks the Axes what to store instead of assuming the view limits are sufficient.

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

fariza commented Jul 26, 2015

I see... In that case why not call it get_view and set_view and move the default there instead of rising an exception?

@QuLogic
Copy link
Member Author

QuLogic commented Jul 26, 2015

get_ and set_ have an existing meaning that I'd like to avoid implying (like set_xlim/get_xlim); the "view" is supposed to be opaque to anything outside the Axes. Taking a cue from Transform, maybe something like freeze_view/unfreeze_view?

@efiring
Copy link
Member

efiring commented Jul 27, 2015

I like save/restore better than freeze/unfreeze; it's a better description of what is happening.

@OceanWolf
Copy link
Contributor

What about push and pop? As I see it we basically deal with a view stack of states...

@fariza
Copy link
Member

fariza commented Jul 27, 2015

Regarding the names

  • save: Means to actually save something, in this case, the method is just returning a specific configuration (limits in the default case), it is not saving anything.
  • restore: means that it was this way before and we are putting it back, this is not true, the view can be completely made up
  • push and pop: implies that we are moving things inside a stack (this is not the case).
  • freeze and unfreeze: Similar to push and pop, it means that the state is stored inside the object. This is not the case.
  • get and set: You actually get the current configuration (view limits and stuff) and you actually set the configuration (view limits and stuff). Where you hold the information is another matter, it can be completely made up or in the most common case this information is stored inside the backend_tools.ToolViewsPositions object

If the methods are going to be inside the axes then, it should be possible to call them externally without having to go through the toolbar. In that case, they have to implement the default behavior and their names have to represent what it is actually doing.

@OceanWolf
Copy link
Contributor

@fariza ahh yes, my mistake. we do use a stack, but the stack lies in the tool. So I agree that it looks the best out of the options presented, however I feel it a bit overkill if we generalise this... say in the future we had a state of 100 variables, then we store 100 changes even if we only changed one of them.

Maybe if the tool just stored a dictionary of the changes?

@fariza
Copy link
Member

fariza commented Jul 27, 2015

@OceanWolf The variables are stored inside the ToolViewsPositions.view dictionnary, so it should be transparent no matter what we are storing inside.

@QuLogic
Copy link
Member Author

QuLogic commented Jul 28, 2015

Now updated with get/set and an API for doing zoom-to-rect. It should now be possible for custom Axes to implement the zoom button just like they can implement the pan/zoom button.


lastx, lasty, x, y = bbox

x0, y0, x1, y1 = original_view
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 really sure why it's necessary to use the frozen view here. Unlike pan/zoom mode, with zoom-to-rect, there's only one time that the view can change (when the button's released), so I wonder if I can just use the existing view limits here?

Copy link
Member

Choose a reason for hiding this comment

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

@tacaswell before merging this, can you confirm that this doesn't have any bad side effects?
I have tested it, and it works fine, but I'm not sure that frozen isn't necessary.

@QuLogic
Copy link
Member Author

QuLogic commented Jul 28, 2015

PS, is there an automated way of testing this sort of thing? I just ran examples/api/two_scales.py.

@fariza
Copy link
Member

fariza commented Jul 28, 2015

Unfortunately there is no way to automatically test.
With the upcoming MEP27 it will be easier to create a virtual backend and emulate user input. For the time being, just try it as much as you can.

@QuLogic
Copy link
Member Author

QuLogic commented Jul 28, 2015

If that's the case, I'm not sure why Travis failed, as it seems a bit unrelated.

@jenshnielsen
Copy link
Member

The failure is unrelated. I restarted travis

else:
continue

a.set_view_from_bbox((lastx, lasty, x, y), view, direction,
Copy link
Member

Choose a reason for hiding this comment

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

this view variable is not defined

@fariza
Copy link
Member

fariza commented Jul 28, 2015

@QuLogic Did you try it with PolarAxes as you mentionned in the description? I am curious to know if works as expected, or something else needs to be moved to Axes.
If possible, try it with both toolmanager and ToolbarNavigation2

@QuLogic
Copy link
Member Author

QuLogic commented Jul 28, 2015

I've been working on it here. It's a bit hacky and some things are buggy, but the API here seems to work well enough.

@fariza
Copy link
Member

fariza commented Jul 29, 2015

👍

@fariza
Copy link
Member

fariza commented Jul 31, 2015

@OceanWolf @tacaswell what do you guys think?

@tacaswell
Copy link
Member

Sorry, I have my hands full else where and have not been paying attention to this thread at all.

@efiring
Copy link
Member

efiring commented Jul 31, 2015

Attn: @mdboom, @pelson: this is important, and I think it deals with
areas in which you are particularly knowledgeable.

On 8/1/15, Thomas A Caswell notifications@github.com wrote:

Sorry, I have my hands full else where and have not been paying attention to
this thread at all.


Reply to this email directly or view it on GitHub:
#4795 (comment)

@pelson
Copy link
Member

pelson commented Aug 3, 2015

Attn: @mdboom, @pelson: this is important, and I think it deals with areas in which you are particularly knowledgeable.

Thanks @efiring. The proposal makes a lot of sense - 👍 on the axes being able to control what and how the "view" is preserved. My only concerns are about:

  • how much logic went into set_view_bbox - I don't feel like that is something I'd like to repeat in a subclass, and yet I also don't know whether all of that method is useful to non-cartesian axes.
  • the names set_view and get_view. These sound like something that a user could see, and misuse accidentally - I'm not averse to having a private API for this behaviour (ax._get_axes_view / ax._set_axes_view would be fine IMO).

Otherwise, awesome contribution @QuLogic, thanks!

@fariza
Copy link
Member

fariza commented Aug 5, 2015

@pelson I actually prefer the public methods, they allow to easily switch views programatically.

@pelson
Copy link
Member

pelson commented Aug 5, 2015

they allow to easily switch views programatically

Completely. But imagine you were coming to mpl for the first time: there are already way to many public methods. This functionality is superb, and I know it will be well used by cartopy for one, but it isn't in the list of the first 30 methods on an Axes I'd show to a new user...

Alternatively, @shoyer and I talked about the concept of method namespaces such that you could do something like axes.gui.set_view() / axes.gui.get_view(). It seems that Pandas has an implementation of this already (and I have a hacked together proof of concept which I did before knowing about the pandas version.

@tacaswell
Copy link
Member

@pelson Long term I would like to pull all of the plotting methods off of the Axes objects which will help address the issue of information overload. We can then use module-level name spacing to group related functions.

I really need to write up a long document of my thoughts on large-scale API changes.

@fariza
Copy link
Member

fariza commented Aug 5, 2015

@pelson Ok, I agree we have tooooooo many public methods in one place.
Sorry @QuLogic please change the names to private ones _set_view and _get_view

Custom Axes may require additional information than simply the x/y
limits to describe the view. This API will allow the navigation buttons
on the toolbar to function correctly with such Axes.
The main benefit here will be PolarAxes, but there might be other custom
Axes that find this useful.
@QuLogic
Copy link
Member Author

QuLogic commented Aug 6, 2015

how much logic went into set_view_bbox - I don't feel like that is something I'd like to repeat in a subclass, and yet I also don't know whether all of that method is useful to non-cartesian axes.

Think of it this way: there were two copies in the two toolbars, and now there's only one copy in the axes, so I've reduced code redundancy!

@QuLogic
Copy link
Member Author

QuLogic commented Aug 6, 2015

I seem to always get this PS backend failure...

@fariza
Copy link
Member

fariza commented Aug 7, 2015

Is anybody against this?

pelson added a commit that referenced this pull request Aug 27, 2015
ENH: Add API to manage view state in custom Axes.
@pelson pelson merged commit eb53398 into matplotlib:master Aug 27, 2015
@pelson
Copy link
Member

pelson commented Aug 27, 2015

@QuLogic - thanks for this awesome change 👍.
Would you mind submitting another PR to add an entry to the whats new? Thx

@QuLogic
Copy link
Member Author

QuLogic commented Sep 29, 2015

I'm pretty sure this is in the rc for 1.5, though it's tagged as 2.1.

@jenshnielsen
Copy link
Member

@QuLogic Yes if you click a commit you can see which tags it's in. Both of these are in 1.5 rc1

@QuLogic
Copy link
Member Author

QuLogic commented Sep 29, 2015

Oh yes, I know; I just like to keep my milestones straight.

@OceanWolf
Copy link
Contributor

Sorry for those just messaged, hit reply to wrong email, just delete.

@jenshnielsen jenshnielsen modified the milestones: next point release (1.5.0), proposed next point release (2.1) Sep 30, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants