-
-
Notifications
You must be signed in to change notification settings - Fork 7.8k
Fixed ZoomPanBase to work with log plots #4970
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
attn @fariza @OceanWolf Can you two take care of reviewing and merging this? |
@tacaswell , can we hold off for a bit? I just realized that my zooming does not conform to yours. Will submit the correction and test when I have time. Thanks |
@tacaswell
Which one to use? I prefer number 3 (similar to what you posted here). Have code ready, will submit change once convention is agreed upon. |
I have always disliked how zooming happened in matplotlib. I vote for On Thu, Aug 20, 2015 at 10:50 AM, Julien Lhermitte <notifications@github.com
|
Two votes is good for me. Submitted. Tested on this branch with same code mentioned above. (math : new axis position: (xa -/+.5_scl + (.5-xa)_scl, then use matplotlib functions to convert to data, which take care of non-linear plots.) |
I'll check this next week. I'm on vacation :)
|
enjoy! :-) |
@ordirules
As far as I can test, it works fine! 👍 |
Ahh, just seen this, a bit busy right now, I will take a look, out of the options, definitely not option 2. Personally I prefer option 3, but I think option 1 also has merits, I think we can revisit it later though (either RcParam or my personal favourite a toggle tool to set zoom mode. @fariza Type Agg with Camel case. |
Thanks @fariza for the test code. The previous code was me desperately trying to find a way to test it with my limited knowledge :-D. Glad I could contribute something, even though exceedingly trivial :-D @OceanWolf if(self.centered is not None):
xa,ya=0.5,0.5
else:
tranP2A = ax.transAxes.inverted().transform
xa, ya = tranP2A((event.x, event.y)) or something of the sort. It might never be used but could be good to add. Is it worth it? I could also do it if it saves you time. |
@ordirules |
That makes a lot of sense, very good point. Didn't notice that the others inherit from ZoomPanBase. ToolZoom looks redundant with transScale (probably preceded it ;-) ) so it would make sense to clean up the duplicate code and follow the current transformations philosophy, from my understanding of the tutorial. The function could be something like "expandshift(ax,x0,y0,x1,y0)" where x,y are the limits in axes coordinates of the new viewbox. I prefer this over "changeview" or something because it emphasizes the relative nature (and potential dangers of looping through). I think it would also make sense to go deeper and have it part of _AxesBase. The main motivation for this is that it would make life easier for a developer who would want to write their own custom routine without using the GUI. From what I understand (correct me if I'm wrong), but all these inherit from ToolBase which is inherently a GUI feature. Anyway, what do you think? To sort of summarize:
I'll do some reading tonight which may answer my first question (currently busy), but I figured I would ask. Anyway, this is a very good point and I vote for your idea (either case, if it goes in Axes or ZoomPanBase). |
Hi, So I took a look at the _AxesBase class and the only function available seems to be set_xlim etc. I think zooming and panning would be appropriate there, since it is more a feature of the axes and not the GUI. (I could imagine other users using this feature without the GUI) As a suggestion, I would like to propose adding these features to the _AxesBase class: @fariza, @OceanWolf, @tacaswell this is a slightly more fundamental proposed change. What are your inputs? I can make the changes no problem but need to know if it is commensurate with current matplotlib philosophy. thanks! :-) |
Another note, the main reason to add this to the base class is that it requires transformation from axes coordinates to data. The idea would be to allow the user to have a set of routines to change limits in axes coordinates, without worrying about the transformations to data coordinates. I have made the assumption that the only time you want to change limits in axes coordinates is when you pan or zoom, so I've named it zoompan instead of set_axes_lim or something. |
Hmm, before I said I agreed with you, but now not so sure... basically as I see it zoom and pan basically do the same thing, they change the axes limits and they work in a user interactive way... so I think we need to just need a conversion function... perhaps something similar to Then again, with different axes types (e.g. 3d) perhaps it becomes easier to do it in the axes itself as it allows the axes to specify its own implementation, rather than making the tool overly complex, catering for all of the different axes types, @WeatherGod you know more about this... |
Sounds good. I definitely agree with your disagreement with the zoompan idea. :-) Anyway, I want to bring the focus back (sorry for straying away). This was meant to be a patch to fix log-log plots where the zooming currently breaks down and it is currently fixed. How about we make a decision on how to patch it which I believe is one of two options:
I would say to just leave as is for now and open a new pull request that suggests more fundamental changes. If you agree I think we should just commit. What do you think? thanks again for the feedback! |
Didn't work for polar coordinate transforms, so changed the way the transformations are done. Also updated ToolZoom to use the transforms available rather than compute them again. There is still an issue w/ the polar coordinate transformation. (I did not realize that commiting to my github would show up here) Here are the assumptions I make:
As of now, the zooming in polar coordinates isn't perfect with the scroll wheel. Sometimes zooming out with the scroll wheel zooms in. Going through all this code is a learning exercise for me. I apologize for all the writing for such a small patch. Testing code: import matplotlib
matplotlib.use('GTK3Agg')
matplotlib.rcParams['toolbar'] = 'toolmanager'
import matplotlib.pyplot as plt
from mpl_toolkits.mplot3d import Axes3D
import numpy as np
plt.ion()
fig1 = plt.figure(0)
img = np.random.random((100,100))
ax = fig1.add_subplot(111)
fig2 = plt.figure(1)
ax2 = fig2.add_subplot(111)
fig3 = plt.figure(2)
ax3 = fig3.add_subplot(111,polar="True")
#fig4 = plt.figure(3)
#ax4 = fig4.add_subplot(111,projection="3d")
x = np.linspace(10,1000,1000)
y = 1/x**4
N = 1000
x = np.linspace(-N/2,N/2,N)
y = np.linspace(-N/2,N/2,N)
X,Y = np.meshgrid(x,y)
w = np.where(np.sqrt(X**2 + Y**2) < 10**2)
img = np.zeros((N,N))
img[w] = 1
#uncomment the one you want to test, and comment the other
ax.imshow(img)
ax2.loglog(x,y)
ax3.plot(x,y) |
Maybe you are referring to #4795 actually (which having just been merged is probably causing conflicts here)? Probably |
Thanks @QuLogic, I think changing the view from a bounding box is I've tried my best to reconcile my code with @QuLogic, and in the Here are the major two changes performed:
That's about it. Here is a quick description of the math for the zooming coordinate calculations which are later
Finally, the test code I have used has not changed from the previous code. @fariza, so what do you think? |
I should also point out on line 3339 (x0, y0, x1, y1 = original_view) from axes/_base.py I had to flip y0 and x1. @QuLogic did zoom work for you with this reversal? |
Oh, looking at it, you're probably right; these have been swapped. |
scl = 1 | ||
|
||
ax = event.inaxes | ||
xmin, xmax, ymin, ymax = ax._get_view() |
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.
The point of _get_view
/_set_view
is to hide the meaning of the "view" from outside of the Axes
, because it is not necessarily Cartesian. The following calculation based on it should probably be placed in the Axes
.
|
||
# Should not happen | ||
if(scl == 0): | ||
scl = 1. |
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.
Add a warning? or perhaps raise an error
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.
yes, good idea. I'll just print a warning and return. Since it's harmless, I think there's no need to raise any exceptions. However, in that case, the user will see quite a few print statements if they repeat the call with wrong (or even just void) arguments. I'm still new to python (been 4 months, coming from yorick, matlab like open source lang and C), so I'm not completely familiar with the pythonic way of error and warning reporting. Open to recommendations.
@@ -487,14 +487,14 @@ def __init__(self, fig, rect, | |||
self._shared_x_axes.join(self, sharex) | |||
if sharex._adjustable == 'box': | |||
sharex._adjustable = 'datalim' | |||
#warnings.warn( | |||
# warnings.warn( | |||
# 'shared axes: "adjustable" is being changed to "datalim"') |
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 think we should just remove these.
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 just fixed for it to conform with PEP8 but the code is out of scope of this PR.
Maybe make a new PR (and I should probably remove the whitespace PEP8 fix)? @fariza ?
ax = event.inaxes | ||
ax._set_view_from_bbox([event.x, event.y, scl]) | ||
|
||
# first check if last scroll was done within |
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.
These sentences could be combined into one.
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.
re-worded into something more terse.
xzc = (xp*(scl - 1) + xcen)/scl | ||
yzc = (yp*(scl - 1) + ycen)/scl | ||
|
||
bbox = [xzc - xwidth/2./scl, yzc - ywidth/2./scl, |
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.
There's a space at the end here causing PEP8 failures.
Thanks @QuLogic. Travis still failing, I'll need to check tonight. |
I think the last Travis error was just a random fluke. I have restarted |
Fixed ZoomPanBase to work with log plots
Great thanks! I never got to catching and avoiding (by not trying to zoom) the div by zero and overflow exceptions. However, these were not checked for in the original code and it could make sense to have this be a separate PR. Do you think it's worth it? |
Is it easy to reproduce? |
one of them is easy by just zooming in/out way too much. The other I'm not so sure. But I assume by carefully examining it we could probably find a way to test it. |
oh and thanks @jenshnielsen for the fix and notice! |
@ordirules in that case, a new PR is perfect |
Can you ping me on that, I feel curious to see how you do that. |
yeah will do :-) |
Tested this code in a matplotlib 1.4.3 environment and current branch. Fixed ZoomPanBase to work in non-linear plots by changing the scale in the axes coordinates and using the respective tranformation functions: transScale and transLimits transforms.
See tutorial on axes transformations for more details:
http://matplotlib.org/users/transforms_tutorial.html
Note: It did not work in my version 1.4.2. The issue is traceable to the TransScale.transform.inverted() routine expecting a 2 dimensional array rather than 1d array. If applying this to this older version this should be investigated/tested.
Testing code used (use mouse scroll wheel afterwards):