Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork7.9k
FIX: do not let no-op monkey patches to renderer leak out#17560
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
Uh oh!
There was an error while loading.Please reload this page.
Conversation
anntzer left a comment• edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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.
Looks fine, but perhaps this should instead be a method (private or public) on the RendererBase class?
def_draw_disabled(self):return_setattr_cm(**{...})
so that you can just write
withrenderer._draw_disabled(): ...
saving on the duplication? Also this way, at least in theory, Renderer subclasses could customize that behavior as needed (effectively bringing back adryrun-like functionality (#14134), but on a strictly optional basis).
I'm 50/50 on moving it to the base class. On one hand I agree it is much neater. On the other hand, how strongly do we require the renderers to actually be sub-classes? I would say that adding a new method to the semi-public API of the renderer is something we should not do in a patch release and we probably should backport this to 3.2.2 to fix the empty figure bug. |
We basically do per#17159. |
Will add helper function. |
I made it forgiving for now (to backport to 3.2.x) but I'll open a follow-on PR to make it warn for 3.3. |
Uh oh!
There was an error while loading.Please reload this page.
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.
doc needs update (so approval conditional on that) but otherwise looks good
Be forgiving about renderer instances that do not inherit fromRendereBase.
Owee, I'm MrMeeseeks, Look at me. There seem to be a conflict, please backport manually. Here are approximate instructions:
And apply the correct labels and milestones. Congratulation you did some good work ! Hopefully your backport PR will be tested by the continuous integration and merged soon! If these instruction are inaccurate, feel free tosuggest an improvement. |
… renderer leak outMerge pull requestmatplotlib#17560 from tacaswell/fix_noop_tight_bboxFIX: do not let no-op monkey patches to renderer leak out Conflicts:lib/matplotlib/backend_bases.py - trivial conflicting imports - did not back port other changes to tight boxlib/matplotlib/tight_layout.py - ended up not backporting any of the changes
… renderer leak outMerge pull requestmatplotlib#17560 from tacaswell/fix_noop_tight_bboxFIX: do not let no-op monkey patches to renderer leak out Conflicts:lib/matplotlib/backend_bases.py - trivial conflicting imports - did not back port other changes to tight boxlib/matplotlib/tight_layout.py - ended up not backporting any of the changes
…-v3.2.xBackport PR#17560: FIX: do not let no-op monkey patches to renderer …
PR Summary
closes#17542
This issue is that in some cases the no-op monkey patched renderer was getting cached deep in the backend code and the no-op functions were getting used for the actual save which resulted in empty figures. This moves the monkey patching to the call sites so we can use the
_setattr_cmto manage this.PR Checklist