Skip to content
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

Doc/rewrite (and code changes inspired by trying to document it) #17

Merged
merged 20 commits into from
Dec 1, 2023

Conversation

tacaswell
Copy link
Member

Re-wrote the docs to include mpl_gui.registry.


mg.show([fig1, fig2])
import mpl_gui.registry as reg
Copy link
Member

Choose a reason for hiding this comment

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

reg may need some consideration, as this will be the start of a "best practices". I appreciate this is registry, but I wonder if end users will think of it that way, versus a list of GUI windows? If the import were something like mwindows then the below would read mwindows.show() which makes more sense to me than registry.show. But maybe "windows" isn't the right idea either?

Copy link
Member Author

Choose a reason for hiding this comment

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

I agree! As with #16 I think almost all of these names are up for debate.

@tacaswell
Copy link
Member Author

Possible "short" versions (and we can change the module name to be something consistent):

  • flt (FigureLisT and reminds you of plt), fl
  • mfigs, my_figs, myfigs, mf, managed_figs
  • mwindows, mwins, mw
  • fig_reg, freg, fr

The other red-flag is that I got my self confused that FigureRegistry does not live in registry.

Having a conversation about this it was impossible to keep straight with the
two meanings of `show`.

closes matplotlib#16
@tacaswell tacaswell changed the title Doc/rewrite Doc/rewrite (and code changes inspired by trying to document it) Nov 1, 2023
@tacaswell tacaswell mentioned this pull request Nov 1, 2023
@rcomer
Copy link
Member

rcomer commented Nov 1, 2023

Should the module currently known as registry have something like “global” in its name, to distinguish it from the FigureResistry objects?

@jklymak
Copy link
Member

jklymak commented Nov 1, 2023

OK, I guess the use here is to mimic

fig, ax = plt.subplots()
... # do plotting
plt.show()

I almost wish this were mpl_gui.subplots() and mpl_gui.show(). I don't remember why you don't want to do it that way? It's hard to think about what the name would be other than "gui". Registry isn't bad I guess, just very abstract

@tacaswell
Copy link
Member Author

I almost wish this were mpl_gui.subplots() and mpl_gui.show() I don't remember why you don't want to do it that way?

Because if the global state lives at the top-level there is no way to opt out of it! By putting it in a submodule users who do not want the global state to be around can either not import it or put None is sys.modules depending on how cranky they are.

@jklymak
Copy link
Member

jklymak commented Nov 2, 2023

Sure, but... is that really a problem? If they don't want to use it, they don't have to - it's just an empty registry object at that point.

I guess if this goes in mpl proper it could be a side module instead of a sub module.

@tacaswell
Copy link
Member Author

We have a number of issues due to users using the global state without realizing it.

I started down this project with the goal of getting rid of global state and only added it back reluctantly (mostly to prove we could re-implement pyplot-like on top of this). If the net result is to replace on unavoidable bit of global with another bit of global state it is not clear to me that there is actually any upside of the code churn.

If (when?) we pull this in to the main repo I think we would need FigureRegistry, a global singleton instance of said registry, and pyplot in three different modules (pyplot would use the singleton and the singleton would obviously import the FigureRegistry) so users could pick their level of global-state opt in.

@tacaswell
Copy link
Member Author

I took @rcomer 's suggestion and put "global" in the name.

@jklymak
Copy link
Member

jklymak commented Nov 2, 2023

OK, I guess if it makes it into main, it could be matplotlib.gui and matplotlib.figreg, or something, and the global state would be in figreg, and folks would do mfr.subplots() or something like that.

@jklymak
Copy link
Member

jklymak commented Nov 2, 2023

... thought now that I think about it, I forget why we need a registry at all. The original suggestion was to just make subplots etc available as well as figure.

@tacaswell
Copy link
Member Author

We need it for the pyplot-replacement and it seems nice to expose publicly.

I think it would be a nice pattern to have user plotting functions that take in a FigureRegistry to either find by name or fabricate what ever figures they need, something like

fr = FigureRegistry()
plot1(..., fr)
plot2(..., fr)
fr.show()

I made the helper functions at the top level private, but could easily be convinced to put them back. Writing the docs, it felt like I had 3 copies of exactly the same code block and "fixed" it by making one different.

@jklymak
Copy link
Member

jklymak commented Nov 2, 2023

We need it for the pyplot-replacement and it seems nice to expose publicly.

I'm still confused by this - happy to sidebar about it if easier...

Of course we need to keep pyplot around, but why can't it have its own registry and mpl_gui remain registry free?

Previously you were offering fig = mpl_gui.figure() and, if I recall correctly, no registry. I suggested it would be nice to have a helperfig, axs = mpl_gui.subplots() (and subplots_mosaic and whatever other shortcuts there. are), which is just an extra call to fig.subplots(). I'm forgetting why that morphed into needing a registry?

 fr = FigureRegistry()
 plot1(..., fr)

What is the advantage of that over just allowing such libraries to continue to use pyplot? Better is surely plot1(..., fig=None) or plot1(..., ax=None) unless you want the methods to know about the Registry for some reason?

@tacaswell
Copy link
Member Author

Going to go ahead and merge this (to get built docs) and continue in a new PR.

@tacaswell tacaswell merged commit aec794a into matplotlib:main Dec 1, 2023
7 checks passed
@tacaswell tacaswell deleted the doc/rewrite branch December 1, 2023 22:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants