NotificationsYou must be signed in to change notification settings
Fork7.9k
Star21.4k

FIX/API:`fig.canvas.draw` always updates internal state#18408

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Merged

QuLogic merged 3 commits intomatplotlib:masterfromtacaswell:fix_pgf_draw

Feb 17, 2021

Merged

FIX/API:`fig.canvas.draw` always updates internal state#18408

QuLogic merged 3 commits intomatplotlib:masterfromtacaswell:fix_pgf_draw

Feb 17, 2021

Conversation

Copy link

Member

tacaswell commentedSep 4, 2020

Previously the non-interactive backends, other than Agg, did not
definedraw methods and fell back to the base no-op version.
However, we have been documenting that the correct way to update the
various internal state we keep (run tight/constrained layouts, auto
limits, text size/position, ...), this is now a bug due to our
suggested usage drifting.

closes#18407

I could be convinced that this should actually be backported for 3.3.2

I'm mixed if these needs an API change note or not.

PR Summary

PR Checklist

Has pytest style unit tests (andpytest passes).
IsFlake 8 compliant (runflake8 on changed files to check).
Conforms to Matplotlib style conventions (installflake8-docstrings andpydocstyle<4 and runflake8 --docstring-convention=all).

tacaswell added this to thev3.4.0 milestone

Sep 4, 2020

QuLogic reviewed

Sep 10, 2020

View reviewed changes

lib/matplotlib/backends/backend_pdf.py Outdated

Comment on lines 2703 to 2732

		def draw(self, args, *kwargs):
		_no_op_draw(self.figure)
		return super().draw(args, *kwargs)

Copy link

Member

QuLogicSep 10, 2020

Choose a reason for hiding this comment

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

I guess the superclass does nothing? It's a bit weird to do a no-op draw, and then go to the super's draw (which, from just looking at this one spot, could conceivably doanything.)

Copy link

MemberAuthor

tacaswellSep 23, 2020

Choose a reason for hiding this comment

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

In 99.9% of cases, the super-call will do nothing, but I do not want to assume that no one has done some (ill-advised?) multiple inheritance of our classes and was relying on hitting their draw.

That said, I think the name_no_op_draw is bad because it is doing an operation (updating our internal state), just not producing any output.

tacaswell force-pushed thefix_pgf_draw branch 2 times, most recently from68fd2fe to23a306fCompare

September 23, 2020 15:33

tacaswell mentioned this pull request

Oct 14, 2020

ticklabels empty when not interactive#6103

Closed

tacaswell force-pushed thefix_pgf_draw branch from23a306f toc078356Compare

October 16, 2020 22:36

QuLogic added Release critical

For bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions.

status: needs rebase labels

Jan 21, 2021

Copy link

Member

QuLogic commentedJan 21, 2021

This needs a rebase, and its corresponding issue is release critical, so marking the same.

tacaswell force-pushed thefix_pgf_draw branch fromc078356 to81e3c11Compare

January 22, 2021 18:43

Copy link

MemberAuthor

tacaswell commentedJan 22, 2021

rebased.

tacaswell removed the status: needs rebase label

Jan 22, 2021

jklymak reviewed

Jan 24, 2021

View reviewed changes

lib/matplotlib/testing/__init__.pyShow resolvedHide resolved

jklymak added the status: needs revision label

Jan 27, 2021

tacaswell force-pushed thefix_pgf_draw branch 4 times, most recently from76f95f7 to3692dedCompare

February 2, 2021 20:04

tacaswell removed the status: needs revision label

Feb 3, 2021

QuLogic reviewed

Feb 9, 2021

View reviewed changes

lib/matplotlib/backends/backend_pdf.py OutdatedShow resolvedHide resolved

lib/matplotlib/tests/test_backend_pgf.pyShow resolvedHide resolved

lib/matplotlib/tests/test_backend_pgf.py OutdatedShow resolvedHide resolved

lib/matplotlib/tests/test_backend_bases.py OutdatedShow resolvedHide resolved

QuLogic added the status: needs rebase label

Feb 10, 2021

tacaswell force-pushed thefix_pgf_draw branch from3692ded to460ff37Compare

February 11, 2021 05:13

anntzer reviewed

Feb 11, 2021

View reviewed changes

lib/matplotlib/backends/backend_pdf.pyShow resolvedHide resolved

Copy link

Contributor

anntzer commentedFeb 11, 2021

Perhaps the docstrings in backend_bases and backend_template should be updated to tell third-party implementers that they need to ensure that draw() walks the artist tree?

anntzer approved these changes

Feb 11, 2021

View reviewed changes

Copy link

Contributor

anntzer left a comment

Choose a reason for hiding this comment

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

Minor comments, but basically good.

QuLogic removed the status: needs rebase label

Feb 12, 2021

QuLogic approved these changes

Feb 12, 2021

View reviewed changes

tacaswell added3 commits

February 15, 2021 20:25

FIX/API:fig.canvas.draw always updates internal state

b7ccfb6

Previously the non-interactive backends, other than Agg, did notdefine `draw` methods and fell back to the base no-op version.However, we have been documenting that the correct way to update thevarious internal state we keep (run tight/constrained layouts, autolimits, text size/position, ...), this is now a bug due to oursuggested usage drifting.closesmatplotlib#18407

MNT: hoist latex helpers from pgf tests to matplotlib.testing

1173770

Move the marks to skip if various latex installs are missing to bevisible on other modules.

DOC: add notes to reference draw methods

22e84f6

The full artist tree needs to be walked in `draw` to ensure thatdeferred work is done.

tacaswell force-pushed thefix_pgf_draw branch from90efb61 to22e84f6Compare

February 16, 2021 01:26

QuLogic merged commit73b5101 intomatplotlib:master

Feb 17, 2021

tacaswell deleted the fix_pgf_draw branch

February 17, 2021 02:41

This was referencedFeb 17, 2021

Request: for non-interactive backends make fig.canvas.draw() force the render#16558

Closed

WIP: for non-interactive backends make fig.canvas.draw() force the render#16573

Closed

tacaswell mentioned this pull request

Mar 28, 2021

ENH: use canvas renderer in draw#19765

Closed

7 tasks

tacaswell mentioned this pull request

Jun 1, 2023

canvas.draw() does not render tick labels with pdf backend#13513

Closed

Labels

Release critical

For bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions.

4 participants

Movatterモバイル変換

Navigation Menu

Search code, repositories, users, issues, pull requests...

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

FIX/API:`fig.canvas.draw` always updates internal state#18408

FIX/API:`fig.canvas.draw` always updates internal state#18408

Uh oh!

Conversation

tacaswell commentedSep 4, 2020

PR Summary

PR Checklist

Uh oh!

QuLogicSep 10, 2020

Choose a reason for hiding this comment

Uh oh!

tacaswellSep 23, 2020

Choose a reason for hiding this comment

Uh oh!

QuLogic commentedJan 21, 2021

Uh oh!

tacaswell commentedJan 22, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

anntzer commentedFeb 11, 2021

Uh oh!

anntzer left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Movatterモバイル変換

Uh oh!

FIX/API:fig.canvas.draw always updates internal state#18408

FIX/API:fig.canvas.draw always updates internal state#18408

Uh oh!

Conversation

tacaswell commentedSep 4, 2020

PR Summary

PR Checklist

Uh oh!

QuLogicSep 10, 2020

Choose a reason for hiding this comment

Uh oh!

tacaswellSep 23, 2020

Choose a reason for hiding this comment

Uh oh!

QuLogic commentedJan 21, 2021

Uh oh!

tacaswell commentedJan 22, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

anntzer commentedFeb 11, 2021

Uh oh!

anntzer left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

FIX/API:`fig.canvas.draw` always updates internal state#18408

FIX/API:`fig.canvas.draw` always updates internal state#18408