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 python repl rd2 #4506

Merged
merged 6 commits into from Jul 16, 2015
Merged

Conversation

tacaswell
Copy link
Member

This and #4503 should fix all of the problems discovered in b2fbae7

attn @efiring @mdehoon

Closes #4504

The IPython hook we use in post_execute, not display.
If we do not have IPython, then the only hook I know of in the repl is
the display hook which is called when ever something is going to be
displayed, not after the execution of the last command has finished (the
reason it exists is to give users a chance to intercept and change the
display of the output, not as a generic call back as we were trying to
use it).  In this case register a callback on the `Figure` artist at
creation time to call `draw_idle` on the canvas if the Figure is stale.

We are guaranteed to see this at least once when stale is flipped to
True, but may see it more than once if other paths call `pchanged()`.
Fortunately, `pchanged` is not widely used in the internal code base and
even if `draw_idle` is called multiple times, we should be able to count
on the backends handling multiple draw_idle commands correctly.

Closes matplotlib#4504
Avoids a possibly un-needed re-draw.
@mdboom
Copy link
Member

mdboom commented Jun 8, 2015

👍

def auto_draw(fig):
if fig.stale and matplotlib.is_interactive():
fig.canvas.draw_idle()
figManager.canvas.figure.add_callback(auto_draw)
Copy link
Member

Choose a reason for hiding this comment

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

should we record the callback id somewhere, maybe to make it possible to disable the auto_draw() if plt.ioff() is called?

Copy link
Member Author

Choose a reason for hiding this comment

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

As this is registered on a per-figure basis that gets a bit messy book keeping wise and would probably need to be pushed back up into Gcf which is more complicated than I really want to make this. I am happy to say that if you really care about controlling ion/ioff, use IPython where we have better control of the hooks.

In the base case, all new figures will respect the setting, existing figures will respect the last setting which seems fair-ish.

I also don't see the use case for rapidly switching between ion/ioff.

@tacaswell
Copy link
Member Author

Failures are pickles not being sanitized enough on the way out (the local function is not pickleable). Should probably fix that, but the faster solution is to move the auto_redraw function to be a top level function (and thus be pickable).

@tacaswell tacaswell added this to the next point release milestone Jun 8, 2015
@tacaswell tacaswell added the Release critical For bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions. label Jun 8, 2015
@efiring
Copy link
Member

efiring commented Jun 8, 2015

I did a little quick testing, and it looks great! Is there any real disadvantage to making auto_redraw top-level?

@tacaswell
Copy link
Member Author

Nope, just me solving this in the first way that popped into my head on the train. I was thinking this was going to be a closure, but it is not.

@tacaswell
Copy link
Member Author

Not sure what is wrong with the build, it seems to build fine locally, but is stalling on travis.

@tacaswell
Copy link
Member Author

sorry ^doc build

@jenshnielsen
Copy link
Member

No that's puzzling. It looks like both jobs hung in or after the same example: examples/mplot3d/rotate_axes3d_demo

@WeatherGod
Copy link
Member

now, that's interesting. rotate_axes3d_demo.py is a bit of an oddball. It and wire3d_animation_demo.py are the only two doc build examples that calls plt.ion(). There is also widgets/lasso_selector_demo.py, but I think the doc-build skips that one.

@tacaswell
Copy link
Member Author

Ah, should try to run it locally with the rcparams that are used on Travis.
Failing to see why draw idle would be a problem with agg tough.

On Tue, Jun 9, 2015, 10:57 Benjamin Root notifications@github.com wrote:

now, that's interesting. rotate_axes3d_demo.py is a bit of an oddball. It
and wire3d_animation_demo.py are the only two doc build examples that calls
plt.ion(). There is also widgets/lasso_selector_demo.py, but I think the
doc-build skips that one.


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

@tacaswell
Copy link
Member Author

so, I am now very confused as no 3D things are working for me on master either:

Traceback (most recent call last):
  File "/home/tcaswell/source/my_source/matplotlib/lib/matplotlib/backends/backend_qt5.py", line 343, in resizeEvent
    self.draw()
  File "/home/tcaswell/source/my_source/matplotlib/lib/matplotlib/backends/backend_qt5agg.py", line 147, in draw
    FigureCanvasAgg.draw(self)
  File "/home/tcaswell/source/my_source/matplotlib/lib/matplotlib/backends/backend_agg.py", line 475, in draw
    self.figure.draw(self.renderer)
  File "/home/tcaswell/source/my_source/matplotlib/lib/matplotlib/artist.py", line 60, in draw_wrapper
    draw(artist, renderer, *args, **kwargs)
  File "/home/tcaswell/source/my_source/matplotlib/lib/matplotlib/figure.py", line 1122, in draw
    func(*args)
  File "/home/tcaswell/.virtualenvs/dd_3k/lib/python3.4/site-packages/mpl_toolkits/mplot3d/axes3d.py", line 273, in draw
    ax.draw(renderer)
  File "/home/tcaswell/.virtualenvs/dd_3k/lib/python3.4/site-packages/mpl_toolkits/mplot3d/axis3d.py", line 264, in draw
    self.axes.transAxes.transform(peparray[0:2, 0]))
ValueError: Expected 2-dimensional array, got 1

@tacaswell
Copy link
Member Author

and I am now completely flumoxed. I have pulled out the values of peparray above:

This works

In [28]: ax.transAxes.transform(np.array(dd[0:2, 0]))
Out[28]: array([ 67.42554669,  57.0430007 ])

This fails:

In [32]: a = ax.transAxes.transform(np.array(dd[0:2, 0]))
---------------------------------------------------------------------------
ValueError                                Traceback (most recent call last)
ValueError: Expected 2-dimensional array, got 1

This works:

In [36]: a = ax.transAxes.transform([-0.07047539, -0.05412152])

This does not:

In [35]: a = ax.transAxes.transform(np.array([-0.07047539, -0.05412152]))
---------------------------------------------------------------------------
ValueError                                Traceback (most recent call last)
ValueError: Expected 2-dimensional array, got 1

and

In [38]: a = ax.transAxes.transform(list(dd[0:2, 0]))

In [39]: a = ax.transAxes.transform(dd[0:2, 0])
---------------------------------------------------------------------------
ValueError                                Traceback (most recent call last)
ValueError: Expected 2-dimensional array, got 1

In [40]: 

For those playing along at home:

In [37]: dd
Out[37]: 
array([[-0.07047539,  0.01976701],
       [-0.05412152, -0.08365497],
       [-0.99055809, -1.03688574]])

and

fig = plt.figure()
ax = fig.add_subplot(111, projection='3d')

@tacaswell
Copy link
Member Author

Sorry, I hope no one wasted any time on this.

The root cause seems to be crossed installations on my end, sorry for the noise.

@tacaswell
Copy link
Member Author

Removing the plt.ion() from those demos fixed the build. I should probably figure out what awful subtle thing is going wrong...

@tacaswell
Copy link
Member Author

I still need to deal with @WeatherGod 's comment about the order of calling plt.ion and creating a figure and the comment I dismissed above about keeping track of the id's of the call backs to remove them when plt.ioff() is called (I was mostly being lazy, sorry Ben).

tacaswell referenced this pull request Jul 12, 2015
There is still one `draw_if_interactive` left in the `rcdefaults`
call as the current scheme does not track when rcparams change.
@efiring
Copy link
Member

efiring commented Jul 12, 2015

@mdehoon, @tacaswell, I have tested this with Python 2.7 (Homebrew) and macosx backend, using the python -i test.py method. It works, but there is a delay of about 5 seconds from the time the window is displayed until the plot is drawn and the terminal cursor returns. With qt4agg and tkagg there is little or no delay. (Tk actually seems snappier than qt4agg, but it's not a big difference.) @mdehoon, where do you think the 5-s delay is coming from? Is there an easy way to avoid it?

Side note: with interactive : True in matplotlibrc, standard mpl scripts ending with plt.show() don't work; because show doesn't block in interactive mode, running such a script from the command line in the normal way, without an explicit python -i, will just flash the plot on the screen. This suggests to me that having the interactive : True option in matplotlibrc is a bad design, so perhaps the matplotlibrc_template should include a comment warning against it. I suspect we've avoided bug reports about this only because few people use their own matplotlibrc file; probably most mpl users are blissfully unaware of it, fortunately.

@mdehoon
Copy link
Contributor

mdehoon commented Jul 14, 2015

@efiring OK I will have a look.

fig : Figure
A figure object which is assumed to be associated with a canvas
"""
if fig.stale and matplotlib.is_interactive():
Copy link
Contributor

Choose a reason for hiding this comment

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

Why limit this only to interactive backends?

Copy link
Member Author

Choose a reason for hiding this comment

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

because if we are not interactive the user does not get updates? This is just mimicking the existing pyplot behavior of draw_if_interactive.

Copy link
Contributor

Choose a reason for hiding this comment

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

"does not get updates" = "cannot update"? Well if we assume that, then we just add an unnecessary check....

However consider tools, if we add a tool that calls my_line2d.set_color('purple'), this line means you need to add a canvas.draw_idle() after that in the tool, but if you run in interactive mode, you don't need it. This feels like inconsistent behaviour... but happy to get persuaded why we don't/may not want that.

@mdehoon
Copy link
Contributor

mdehoon commented Jul 16, 2015

@efiring

where do you think the 5-s delay is coming from? Is there an easy way to avoid it?

This turned out to be a trivial problem. The MacOSX backend doesn't implement the draw_idle function, so matplotlib was calling draw_idle in the base class. However, the required functionality is available in the C extension, where it's called "invalidate" (this is what is currently used in draw_if_interactive). If I add a draw_idle method to FigureCanvasMac and let it call self.invalidate(), then everything seems to work fine.

@mdehoon
Copy link
Contributor

mdehoon commented Jul 16, 2015

Perhaps related to @OceanWolf 's question: What is the purpose of using the callback functions _stale_figure_callback and _stale_axes_callback instead of setting figure.stale and axes.stale directly in stale.setter?

@efiring
Copy link
Member

efiring commented Jul 16, 2015

@tacaswell, I had the same question as @mdehoon raised above; including a comment in the code to explain it might be helpful.
@mdehoon, if you have not already done so, would you submit a PR with the draw_idle method, please? Thanks.

@tacaswell
Copy link
Member Author

It is a combination of the terrifying things that we do with the axes attribute and forward planning.

For 'normal' Artists it is the axes they are in, for Axes objects it if forbidden, and for Figure objects it is a list of the Axes it contains. If I recall correctly, I think I thought doing it with the callbacks was simpler than special-casing the stale property.

The other part is looking forward to what I want to do with containers, being able to do the dispatch to parent via the call back stack allows artists to notify their container (which will in turn notify the axes) that they are stale. Being able to keep track of which parts of the tree are stale might provide a path to auto-blitting in the Agg backends (as we know exactly which artists we need to re-render and which we can used cached versions of), but that is still vapor ware.

@tacaswell
Copy link
Member Author

Side note: with interactive : True in matplotlibrc, standard mpl scripts ending with plt.show() don't work; because show doesn't block in interactive mode, running such a script from the command line in the normal way, without an explicit python -i, will just flash the plot on the screen.

Did I break that or has that always been the case?

@efiring
Copy link
Member

efiring commented Jul 16, 2015

@tacaswell, it's that way in master, and probably has always been that way, or at least has been in the modern era. It seems pretty fundamental.

@efiring
Copy link
Member

efiring commented Jul 16, 2015

@tacaswell, I'm inclined to merge this now; is there any reason I should wait?

@tacaswell
Copy link
Member Author

I have been meaning to go back and re-work how the plain python shell
ion/ioff work. If you toggle them any existing figures will still
(not)have the callback on them so will persist with the previous behavior,
but all new figures will work properly.

It isn't hard, just some book keeping. If you want to merge this now I can
add that in as another PR.

On Thu, Jul 16, 2015 at 5:07 PM Eric Firing notifications@github.com
wrote:

@tacaswell https://github.com/tacaswell, I'm inclined to merge this
now; is there any reason I should wait?


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

efiring added a commit that referenced this pull request Jul 16, 2015
@efiring efiring merged commit 73ae7f9 into matplotlib:master Jul 16, 2015
@efiring
Copy link
Member

efiring commented Jul 16, 2015

Merged, so that we can get more people banging on it.

@tacaswell tacaswell deleted the enh_python_repl_rd2 branch July 16, 2015 22:43
@mdehoon
Copy link
Contributor

mdehoon commented Jul 17, 2015

@efiring

if you have not already done so, would you submit a PR with the draw_idle method, please? Thanks.

Please see pull request #4731 .

@jorgesca
Copy link

This morning I updated matpotlib master and then installed trendvis I got an ValueError: Expected 2-dimensional array, got 1 error when trying one of the examples. Searching I found #3898 which led me here. Tried with one the mplot3d demos, lines3d_demo and got the same error:

$ python lines3d_demo.py 
/home/jscandal/sw/python/doct/ciarp07/lines3d_demo.py
Traceback (most recent call last):
  File "/home/jscandal/sw/matplotlib/matplotlib.py3/lib/matplotlib/backends/backend_qt5agg.py", line 74, in paintEvent
    FigureCanvasAgg.draw(self)
  File "/home/jscandal/sw/matplotlib/matplotlib.py3/lib/matplotlib/backends/backend_agg.py", line 471, in draw
    self.figure.draw(self.renderer)
  File "/home/jscandal/sw/matplotlib/matplotlib.py3/lib/matplotlib/artist.py", line 60, in draw_wrapper
    draw(artist, renderer, *args, **kwargs)
  File "/home/jscandal/sw/matplotlib/matplotlib.py3/lib/matplotlib/figure.py", line 1121, in draw
    func(*args)
  File "/usr/lib/python3.4/site-packages/mpl_toolkits/mplot3d/axes3d.py", line 273, in draw
    ax.draw(renderer)
  File "/usr/lib/python3.4/site-packages/mpl_toolkits/mplot3d/axis3d.py", line 264, in draw
    self.axes.transAxes.transform(peparray[0:2, 0]))
ValueError: Expected 2-dimensional array, got 1
Traceback (most recent call last):
  File "/home/jscandal/sw/matplotlib/matplotlib.py3/lib/matplotlib/backends/backend_qt5agg.py", line 74, in paintEvent
    FigureCanvasAgg.draw(self)
  File "/home/jscandal/sw/matplotlib/matplotlib.py3/lib/matplotlib/backends/backend_agg.py", line 471, in draw
    self.figure.draw(self.renderer)
  File "/home/jscandal/sw/matplotlib/matplotlib.py3/lib/matplotlib/artist.py", line 60, in draw_wrapper
    draw(artist, renderer, *args, **kwargs)
  File "/home/jscandal/sw/matplotlib/matplotlib.py3/lib/matplotlib/figure.py", line 1121, in draw
    func(*args)
  File "/usr/lib/python3.4/site-packages/mpl_toolkits/mplot3d/axes3d.py", line 273, in draw
    ax.draw(renderer)
  File "/usr/lib/python3.4/site-packages/mpl_toolkits/mplot3d/axis3d.py", line 264, in draw
    self.axes.transAxes.transform(peparray[0:2, 0]))
ValueError: Expected 2-dimensional array, got 1

@WeatherGod
Copy link
Member

Did you follow the suggestion by @tacaswell ? (completely clean out previous installs of matplotlib/mpl_toolkits, then a git clean -fxd, then install). This PR wouldn't cause the issue you are seeing, but mismatched installs of mpl_toolkits and matplotlib would.

@OceanWolf
Copy link
Contributor

@jorgesca Have you rebuilt the master branch? Working a bit on tests the past few days and I have run into problems needing to rebuild the c libraries...

python setup.py develop (or install depending on your preference)

@jorgesca
Copy link

@WeatherGod and @OceanWolf Yes, I think I have followed the suggestion by @tacaswell , but will try once more in case a made a mistake. I'll report back soon

@tacaswell
Copy link
Member Author

You definitly have more than one version of mpl installed as

 File "/home/jscandal/sw/matplotlib/matplotlib.py3/lib/matplotlib/backends/backend_qt5agg.py", line 74, in paintEvent
    FigureCanvasAgg.draw(self)

vs

  File "/usr/lib/python3.4/site-packages/mpl_toolkits/mplot3d/axes3d.py", line 273, in draw
    ax.draw(renderer)
  File "/usr/lib/python3.4/site-packages/mpl_toolkits/
``

There is some issue which I do not fully understand related to how the namespace packages are handled that puts them in different places.  I strongly suggest using virtual environments to keep things isolated from system python. 

@jorgesca
Copy link

I must have done something wrong before, because the lines3d_demo works now. However, the examples from trendvis continue to raise the same error. Should I open a new issue with the details? I realize now my comments are on the wrong place, I apologize.

@tacaswell
Copy link
Member Author

Yes, please open a new issue with this.

On Tue, Jul 28, 2015 at 2:33 PM Jorge Scandaliaris notifications@github.com
wrote:

I must have done something wrong before, because the lines3d_demo works
now. However, the examples from trendvis continue to raise the same error.
Should I open a new issue with the details? I realize now my comments are
on the wrong place, I apologize.


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

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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

issue with display hook in base python repl
8 participants