Skip to content

Enh new figure function in backends #18852

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

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

tacaswell
Copy link
Member

PR Summary

Code arising out of a discussion on gitter, still needs a bunch of documentation and to sort out if we actually need to track the GUI windows (which I was asserting we did, but am no longer sure that is true).

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).

The docstring in ManagerBase claims that *num* can be str or int,
however ever backend used int-specific formatting so this was not
actually true.

Fixed the backends rather than fixing the docstring (because setting
the window title on Manager init is useful and implicitly supported
through some special-casing in `plt.figure`).
The previous code only sets the title once, on refresh we would fall
back to using the result of `fig.get_label()` if in is not the empty
string or f'Figure {num}'.
@tacaswell tacaswell added this to the v3.4.0 milestone Oct 31, 2020
@tacaswell tacaswell requested review from anntzer and jklymak October 31, 2020 00:13
@tacaswell
Copy link
Member Author

I think things that could be added:

  • a close method on Figure that forwards through to the manager like show does
  • a get_me_a_working_GUI_backend function that can be exposed (but not auto-called!) in matplotlib.__init__
import matplotlib as mpl
new_figure = mpl.get_me_a_working_figure_factory()
fig = new_figure('a label')
ax_dict = fig.subplots_mosaic('AA\nBC')

@anntzer
Copy link
Contributor

anntzer commented Oct 31, 2020

See also #14024? I think in any case you need to change show to make such figures showable (unless I'm mistaken, in your PR, as it is, figures can't be shown).

@jklymak
Copy link
Member

jklymak commented Oct 31, 2020

import matplotlib as mpl
new_figure = mpl.get_me_a_working_figure_factory()
fig = new_figure('a label')
ax_dict = fig.subplots_mosaic('AA\nBC')

OK, but why can't this be spelled:

import matplotlib as mpl
fig = mpl.figure()
ax_dict = fig.subplots_mosaic('AA\nBC')

The user doesn't need to know that this factory is spooled up? I appreciate that makes some global state, but in __init__ that could just be: _figure_factory = None until the first call to mpl.figure().

@tacaswell
Copy link
Member Author

On the default branch fig.show() / fig.canvas.manager.show() works (but assumes you have done %gui qt or are managing the event loop on your own). There is currently a show method on Figure that just forwards to manager.show(). I agree with @ImportanceOfBeingErnest 's comments in #14024 that we should not make Figure.show run the event loop (to keep it closer to the semantics of a GUI framework).

linking to a conversation with @jklymak on gitter: https://gitter.im/matplotlib/matplotlib?at=5f9dc6c02a35440715f8e6a3

The lesson I have learned from working on Matplotlib is that global state is a giant headache as is doing GUI integration. I am very 👍 on us keeping the code to do that for the user, but I am not 100% sure we got everything right so am concerned about always calling that code for the user. I think it is important that we maintain a way for people who want to do their own thing to be sure that they will never have to deal with our (likely still buggy) attempts to "do the right thing" have a way to do that (by not importing the module where we act like an application rather than a library).

There definitely needs to be a way

@tacaswell tacaswell modified the milestones: v3.4.0, v3.5.0 Jan 14, 2021
@tacaswell
Copy link
Member Author

I don't have the bandwidth to push this across the line for 3.4.

@tacaswell tacaswell modified the milestones: v3.5.0, v3.6.0 Aug 5, 2021
@tacaswell
Copy link
Member Author

Pushing to 3.6

@timhoffm timhoffm modified the milestones: v3.6.0, unassigned Apr 30, 2022
@story645 story645 modified the milestones: unassigned, needs sorting Oct 6, 2022
@github-actions
Copy link

github-actions bot commented Sep 4, 2023

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.

@github-actions github-actions bot added the status: inactive Marked by the “Stale” Github Action label Sep 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: inactive Marked by the “Stale” Github Action status: needs rebase
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants