Skip to content

ENH: use canvas renderer in draw #19765

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

Closed
wants to merge 1 commit into from

Conversation

jklymak
Copy link
Member

@jklymak jklymak commented Mar 24, 2021

PR Summary

I'm not sure why we required the renderer to be passed to fig.draw, but suggest we allow it to be a kwarg and default to fig.canvas.get_renderer()

PR Checklist

  • Has pytest style unit tests (and pytest passes).
  • Is Flake 8 compliant (run flake8 on changed files to check).
  • New features are documented, with examples if plot related.
  • Documentation is sphinx and numpydoc compliant (the docs should build without error).
  • Conforms to Matplotlib style conventions (install flake8-docstrings and run flake8 --docstring-convention=all).
  • New features have an entry in doc/users/next_whats_new/ (follow instructions in README.rst there).
  • API changes documented in doc/api/next_api_changes/ (follow instructions in README.rst there).

@jklymak jklymak added this to the v3.5.0 milestone Mar 24, 2021
@jklymak jklymak force-pushed the enh-use-default-renderer branch from 91e97fa to 3f7d454 Compare March 24, 2021 21:18
@tacaswell
Copy link
Member

I am 👎 on this.

Every artist has a draw method that requires a renderer to be passed in and I am skeptical of making the figure draw special.

We already have savefig on the Figure and , draw and draw_idle on the Canvas which is indisputably confusing, but I do not think that re-purposing Figure.draw to also be a user-facing will improve things (and might make it worse as there will be one more way that "works").

@jklymak
Copy link
Member Author

jklymak commented Mar 25, 2021

What is the user-facing way to force a draw on a figure if not Figure.draw()? Maybe I'm just confused, but thats usually how I trigger a draw, except I have to do a song and dance to get the renderer.

@tacaswell
Copy link
Member

fig.canvas.draw()

@jklymak
Copy link
Member Author

jklymak commented Mar 26, 2021

I think the interplay between a figure and its canvas is pretty obscure to most people, including me apparently. I still think a discoverable way to trigger a draw is preferable, but don't feel super strongly about it.

@jklymak jklymak closed this Mar 26, 2021
@jklymak jklymak deleted the enh-use-default-renderer branch March 26, 2021 00:03
@jklymak jklymak restored the enh-use-default-renderer branch March 26, 2021 14:31
@jklymak
Copy link
Member Author

jklymak commented Mar 26, 2021

Actually I'll re-open. We do need to tell users to trigger draws, and having to send them to something that is undocumented is not great. I think that trumps aesthetics as to whether figure is a special artist or not. As for other artists, I'd be fine if we wanted them to get their canvas and use that as a default in all these routines...

@jklymak jklymak reopened this Mar 26, 2021
@timhoffm
Copy link
Member

timhoffm commented Mar 27, 2021

Without having looked into the details and without claiming to understand the internal mechanics: draw() feels like a closed operation that does not need parametrization. Are there cases in which we need to pass different renderers?

@anntzer
Copy link
Contributor

anntzer commented Mar 27, 2021

I believe that a problem is that fig.canvas.draw clears the canvas first, whereas artist.draw (including figure.draw doesn't (it can't, of course: its semantics are to draw just itself onto a canvas which may already have other artists drawn).
Usually this doesn't matter, as figure.draw will draw itself over anything that was already present -- but this would fail if the figure uses a transparent background.

@jklymak
Copy link
Member Author

jklymak commented Mar 27, 2021

Hmm, we have fig.draw() which just draws the figure onto the canvas without erasing what was there before. We have fig.canvas.draw which erases the canvas first, and we have plt.draw the uses of which I do not understand.

What I am suggesting here is to change fig.draw to work without a renderer. If that just means calling down to fig.canvas.draw I am fine with that. If we want pyplot.draw to instead call fig.canvas.draw I'd be fine with that.

I agree this is all confusing, but my confusion is that I don't understand all the other needs to call draw, or ion or the other weird ways we have of triggering things.

@timhoffm
Copy link
Member

timhoffm commented Mar 28, 2021

There's (i) fig.canvas.draw and (ii) fig.canvas.draw_idle for drawing the whole canvas. The latter defers the draw to when control is back to the GUI event loop, and it prevents multiple draws piling up during that time. (ii) should call (i).
Then, there's (iii) pyplot.draw which is the same as (ii).
Additionally, you have (iv) Artist.draw(), which is a low-level function that is passed a renderer and draws the specific artist only. I assume that (iv) is called by (i) on the Figure as the outermost artist, but did not follow the code path.

@jklymak
Copy link
Member Author

jklymak commented Mar 28, 2021

Thanks, that all sounds right. Unfortunately plt.draw() and fig.canvas.draw_idle() do not trigger a draw mid-script, so they are not useful for cases where you need to force a draw to determine where something is on the canvas. i.e.

import matplotlib.pyplot as plt

fig, ax = plt.subplots(constrained_layout=True)

renderer = fig.canvas.get_renderer()
print(ax.get_tightbbox(renderer))  # gives Bbox(x0=96.18055555555554, y0=58.15555555555555, x1=1174.125, y1=855.8)
plt.draw()

print(ax.get_tightbbox(renderer))  # gives same
fig.canvas.draw()
print(ax.get_tightbbox(renderer)) # actually repositions things: Bbox(x0=8.334000000000003, y0=8.334000000000003, x1=1271.666, y1=951.666)

and only the last fig.canvas.draw() actually triggers a draw. I guess this is because the window hasn't been opened yet so the QTimer is not acting on anything, so draw_idle doesn't do anything (the _draw_idle this is supposed to queue up never gets called if the window never opens).

I will still maintain that it is unintuitive that just to get the proper position of elements on the canvas we have to reach down and force a draw on the canvas

@tacaswell
Copy link
Member

I guess this is because the window hasn't been opened yet so the QTimer is not acting on anything, so draw_idle doesn't do anything (the _draw_idle this is supposed to queue up never gets called if the window never opens).

Mid-script you have not given the Qt event loop a chance to run (see https://matplotlib.org/stable/users/interactive_guide.html#scripts-and-functions).

I assume that (iv) is called by (i) on the Figure as the outermost artist, but did not follow the code path.

yes.


To make matters worse there is also Axes.draw_artist and Figure.draw_arist (which redraws a child using a cached renderer and Axes.draw_in_frame (which redraws a sub-selection of the Axes children) and the print_XYZ methods that we use in savefig.

I agree it is confusing that almost everything is named draw despite doing different things, but I still think adding a special case on Figure.draw will only make it worse.

In 3.4 we made sure that fig.canvas.draw also does the right thing for non-interactive backends: #18408


I think what we should do instead is to promote

def _no_output_draw(figure):
renderer = _get_renderer(figure)
with renderer._draw_disabled():
figure.draw(renderer)
to have a public API. Using that we can turn the crank enough to get font sizes / layout / autoscale but it does so without producing any side effects. Something like mpl.make_internal_state_consistent(fig) or fig.resolve_deferred_computations ?

@jklymak
Copy link
Member Author

jklymak commented Mar 28, 2021

I think that would be great plus or minus the name.

@jklymak
Copy link
Member Author

jklymak commented Mar 28, 2021

Fig.no_output_draw doesn't seem so bad to me, or draw_no_output.

@pharshalp
Copy link
Contributor

pharshalp commented Mar 28, 2021

Fig.no_output_draw doesn't seem so bad to me, or draw_no_output.

As a frequent user of constrained_layout, I have been saving figures to a BytesIO object to trigger a draw (since it works even for a non interactive backend like pgf):

with io.BytesIO() as temp_io_buffer:
    fig.savefig(temp_io_buffer)

so replacing all that with a Fig.no_output_draw does sound like a good idea!

@tacaswell
Copy link
Member

So I think the consensus here is:

  • this is a functionality we need
  • shoehorning it into draw is not ideal
  • solve this by adding a new public facing API [name to be not-named-by-tom]

"no_output_draw" is a bit miss leading because the "output" of fig.canvas.draw is all side-effects and dependent on the backend.

What _no_output_draw is doing is disabling all of the draw_XYZ methods (which need to be added to the list of method names that have draw in them!) so that we run the code in the Arist.draw method but the render methods are no-ops (which is both a performance thing and a "managing state" thing as some thing (e.g. the pdf backend) require a live file handle to write to).

I think the name needs to communicate that this:

  • has no side effects outside of the Figure and its children
  • will have many side effects inside of the Figure
  • after running this we expect that:
    • we have correct size estimates of all of the elements (so that the layout tools work)
    • any "auto" scaling / limiting is done (so that we can be
    • the ticks are correct and have the right values in them

@jklymak
Copy link
Member Author

jklymak commented Mar 29, 2021

I'm just a simple fellow, with minimal knowledge of the backends, but that sounds a lot like drawing with "no output" to the screen or file system. I don't think "no output" conveys nothing happens.

@jklymak jklymak marked this pull request as draft March 30, 2021 03:43
@jklymak
Copy link
Member Author

jklymak commented Apr 15, 2021

Replaced by #19967

@jklymak jklymak closed this Apr 15, 2021
@jklymak jklymak deleted the enh-use-default-renderer branch April 15, 2021 14:43
@QuLogic
Copy link
Member

QuLogic commented Apr 15, 2021

But that's also closed?

@jklymak
Copy link
Member Author

jklymak commented Apr 15, 2021

Ooops, #19968 replaces that. I was trying a new GitHub workflow, and obviously didn't understand what it was doing!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants