-
-
Notifications
You must be signed in to change notification settings - Fork 8k
Dont double draw #17923
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
Dont double draw #17923
Conversation
Does this need an API change note?
I think the intention is that this is not really user-visible except for performance.
Yes, but I needed the PR number to write it ;)
Yes, but I needed the PR number to write it ;)
That is a problem with that API re-org structure....
To reduce breakage in user code, would it make sense to change pyplot.draw()
so that if not interactive it calls figure.canvas.draw()
?
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.
slight edit, and some questions
To reduce breakage in user code, would it make sense to change pyplot.draw() so that if not interactive it calls figure.canvas.draw()?
I am wary of adding more special cases here. The new behavior is now much closer to what the GUI backends do (in which draw_idle
schedules a draw the next time the GUI event loops spins at some indeterminate time in the future.
84fdda2
to
ba230ee
Compare
ba230ee
to
d4a34fc
Compare
I like Jody's suggestion to use draw_no_output directly, instead of indirectly via figure.canvas.draw.
Regarding pyplot.draw: the upshot seems to be that it has always been inherently confusing. The docstring implies it is useless--especially as part of pyplot, which is supposed to be the simplest interface. If I understand correctly, this PR makes it even more useless than it was. As in the tests you are changing, what the pyplot user really needs to do is probably draw_no_output, not draw_idle. In that case, this PR is making pyplot.draw more internally consistent, but in all the cases that matter in user code, it is silently making it useless, when before it was useful precisely because of its internal inconsistency.
Pushed this to 3.6 to keep kicking the can of thinking this through down the road.
I tried doing a blind fig.canvas.draw()
-> fig.draw_no_output()
conversion in all of the tests and found 3 cases where we were actually relying on exercising the backend render logic so I am not sure it is safe to do that blind conversion (2 of them were expected failures one actually looked at the RGBA from Agg). I could easily believe that some of the tests are showing that the happy paths are happy.
On the bright side there are only ~150 of them.
Certainly I wasn't suggesting we change all of them, at least here - just the new ones you added in this PR (if they work)!
Yeah, but it was less work to use sed to change them all :-p
This ensures that if the test relied on the draw actually happening, it still happens.
Co-authored-by: Jody Klymak <jklymak@gmail.com>
d27c713
to
7924d7e
Compare
This is probably a bad idea
7924d7e
to
9bd2702
Compare
Since this Pull Request has not been updated in 60 days, it has been marked "inactive." This does not mean that it will be closed, though it may be moved to a "Draft" state. This helps maintainers prioritize their reviewing efforts. You can pick the PR back up anytime - please ping us if you need a review or guidance to move the PR forward! If you do not plan on continuing the work, please let us know so that we can either find someone to take the PR over, or close it.
PR Summary
Replaces #5800
PR Checklist