Skip to content

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.

Sign up for GitHub

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

Jump to bottom
Merged
QuLogic merged 3 commits into from
Feb 17, 2021
Merged

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

QuLogic merged 3 commits into from
Feb 17, 2021

Conversation

@tacaswell
Copy link
Member

@tacaswell tacaswell commented Sep 4, 2020

Previously the non-interactive backends, other than Agg, did not
define draw 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 (and pytest passes).
  • Is Flake 8 compliant (run flake8 on changed files to check).
  • Conforms to Matplotlib style conventions (install flake8-docstrings and pydocstyle<4 and run flake8 --docstring-convention=all).

@tacaswell tacaswell added this to the v3.4.0 milestone Sep 4, 2020
QuLogic
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

@QuLogic QuLogic Sep 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 do anything.)

Copy link
Member Author

@tacaswell tacaswell Sep 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 tacaswell force-pushed the fix_pgf_draw branch 2 times, most recently from 68fd2fe to 23a306f Compare September 23, 2020 15:33
@tacaswell tacaswell mentioned this pull request Oct 14, 2020
Closed
@QuLogic 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
@QuLogic
Copy link
Member

QuLogic commented Jan 21, 2021

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

@tacaswell
Copy link
Member Author

tacaswell commented Jan 22, 2021

rebased.

jklymak
jklymak reviewed Jan 24, 2021
View reviewed changes
@tacaswell tacaswell force-pushed the fix_pgf_draw branch 4 times, most recently from 76f95f7 to 3692ded Compare February 2, 2021 20:04
QuLogic
QuLogic reviewed Feb 9, 2021
View reviewed changes
anntzer
anntzer reviewed Feb 11, 2021
View reviewed changes
@anntzer
Copy link
Contributor

anntzer commented Feb 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
anntzer approved these changes Feb 11, 2021
View reviewed changes
Copy link
Contributor

@anntzer 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.

@tacaswell
FIX/API: fig.canvas.draw always updates internal state
b7ccfb6 
Previously the non-interactive backends, other than Agg, did not
define `draw` 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 matplotlib#18407
@tacaswell
MNT: hoist latex helpers from pgf tests to matplotlib.testing
1173770 
Move the marks to skip if various latex installs are missing to be
visible on other modules.
@tacaswell
DOC: add notes to reference draw methods
22e84f6 
The full artist tree needs to be walked in `draw` to ensure that
deferred work is done.
@QuLogic QuLogic merged commit 73b5101 into matplotlib:master Feb 17, 2021
@tacaswell tacaswell deleted the fix_pgf_draw branch February 17, 2021 02:41
This was referenced Feb 17, 2021
Closed
Closed
@tacaswell tacaswell mentioned this pull request Mar 28, 2021
Closed
7 tasks
@tacaswell tacaswell mentioned this pull request Jun 1, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@anntzer anntzer anntzer approved these changes

@jklymak jklymak jklymak left review comments

@QuLogic QuLogic QuLogic approved these changes

Assignees

No one assigned

Labels

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

Projects

None yet

Milestone

v3.4.0

Development

Successfully merging this pull request may close these issues.

pgf backend no longer supports fig.draw

4 participants

@tacaswell @QuLogic @anntzer @jklymak