On closer look, I can't see what this does?

I can't see original_view getting used anywhere in this function, also x0, x1 and y0, y1 get reset seem to get set immediately in the code below without the values acquired here ever getting used... confused...

On 2015/09/20 2:19 PM, OceanWolf wrote:

I can't see |original_view| getting set anywhere in this function, also
|x0, x1| and |y0, y1| get reset seem to get set immediately in the code
below without the values acquired here ever getting used... confused...

I am, too. @QuLogic, why is original_view needed at all? Or is it
needed only for some projections? It would be nice to have this either
commented or in the docstring.

You know, I think I agree with you as well, mostly. original_view is a parameter, so it's set when the method is called. Nevertheless, x0, x1, y0, y1 do seem to be replaced right after. I think this must be an artifact from some other refactoring of the original zooming method. The same redundant assignment occurs there as well. I had opined on that in #4795, but never really pursued it; now I think I should have looked a little more closely.

Sorry, this is my fault. I have updated the code (regarding the errors and fuzzy logic). I went back and copied and pasted what I originally had put in from my previous pull request. I was asked to move this modification to a new PR.
I had a rough week and dislocated my shoulder, typing with one arm from no sleep and ibuprofen, I apologize....

Hey, no worries, glad to see you contributing, and no, the big issue I have here (and still have) comes from the code already in master, namely that we have code here that does nothing (and you tweak code that does nothing, so...). With team projects such as this, everyone becomes responsible for the code, with everyone able to take a look and review code. We do it as a team effort.

So yes, the dead code. If you look below, x0,and x1; and y0 and y1 get set at the top of each if branch before they get used... which means that original_view does not get used and should get removed from the code, assuming no-one else has any objections...?

Yes I agree, it wasn't your fault @ordirules; git blame shows that the original line existed since 2007 and the sections checking for twin[xy] were added later in 2008. So as surmised earlier, this was dead code from a long-forgotten refactor/enhancement.

In fact, that's where the swapped x1, y0 originally came from.

thanks yeah, good catch. I've made the change. :-)

@ordirules I think you still need to update the callers of _set_view_from_bbox.

@tacaswell: Since this is a cleanup of an API being added to 1.5, I think it should still try to target 1.5 so people don't use the old one. Admittedly, the API is a private one, though.

There are two uses which are now broken:

09:14 $ git grep _set_view_from_bbox
doc/api/api_changes.rst::meth:`~matplotlib.axes._base._AxesBase._set_view_from_bbox`, allow for custom
lib/matplotlib/axes/_base.py:    def _set_view_from_bbox(self, bbox, original_view, direction='in',
lib/matplotlib/backend_bases.py:            a._set_view_from_bbox((lastx, lasty, x, y), view, direction,
lib/matplotlib/backend_tools.py:            a._set_view_from_bbox((lastx, lasty, x, y), view, direction,

Fine with this going in for 1.5.0

ok will make the change.
Just one question on general design. The function currently is preceded by an underscore, which is meant to suggest it should not be accessed from outside the class. Is it okay to access it from backend_*py?
Or is it ok to make it public?

I honestly prefer the latter and would re-write the functions as (while it still has not been used):
get_bbox
set_bbox
since now we're assuming all axes have some kind of rectangular bounding box.

Okay to change? @QuLogic @OceanWolf (and any other expert in this one)
(Will do this tonight, swamped with other stuff)

Since we are already in RC mode, let's keep it private for now. I say, go
ahead with using it in backend_*.py, just make sure that we fix that in 2.1.

On Tue, Sep 29, 2015 at 9:27 AM, Julien Lhermitte notifications@github.com
wrote:

ok will make the change.
Just one question on general design. The function currently is preceded by
an underscore, which is meant to suggest it should not be accessed from
outside the class. Is it okay to access it from backend_*py?
Or is it ok to make it public?

I honestly prefer the latter and would re-write the functions as (while it
still has not been used):
get_bbox
set_bbox
since now we're assuming all axes have some kind of rectangular bounding
box.

Okay to change? @QuLogic https://github.com/QuLogic @OceanWolf
https://github.com/OceanWolf (and any other expert in this one)

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

yes. prefixing with underscore means private, but... of course python doesn't care so much about that, the rule of thumb with accessing private members from outside goes along the lines of:

Don't use unless you have to, and if you do, you do so at your own risk as this code should get considered as highly volatile, i.e. we can change its API without notice and without deprecation.

I like the plan of keeping the private for a while before making them public.

since now we're assuming all axes have some kind of rectangular bounding box.

No, absolutely not; we are not making that assumption at all. The view is an opaque Axes-specific representation of a view; no-one but the Axes knows what it is. Only in the case of a Cartesian axes is it a bounding box, and you are assuming that will not change, but there is no reason why it must stay that way.

The function currently is preceded by an underscore, which is meant to suggest it should not be accessed from outside the class.

It is private because the public should not use it, not because other parts of matplotlib can't.

Thanks for the clarification about the preceding underscore (I assumed the convention was to use it to restrict its use to the class).

@QuLogic thanks for clarifying this. I assumed that since it is possible to change the view with a bbox that there is a one-to-one correspondence between the two. However, I have thought of it and this correspondence is broken if the function ignores the bbox for example, or if there are axes that have voids within the bbox (i.e. what does it mean when selection is in that void), many crazy examples...

This patch is straightforward and is ready for review.

However, I am thinking ahead for the zooming after this and would like to take the opportunity to quickly discuss (if you have time). I think we can incorporate it just fine by simply adding an extra rule to the _set_view_from_bbox function:
if bbox is of len 3 (as opposed to length 4), then treat it as an xc,yc,scl triplet. This will zoom according to scale factor scl around the point xc,yc (in display coordinates).

I have thought of a few scenarios and this is the best fix I can think of. It moves the zooming behaviour to the axes and does not add a new function to the axes (which already have quite a lot). The zoom history can easily be managed to the new get_view and set_view functionalities within the toolbar. I think this integrates quite nicely. It's a trade off of the loss of explicitness for verbosity in my opinion where I'm going for the former.

@ordirules @QuLogic Thanks!