-
Notifications
You must be signed in to change notification settings - Fork 5
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
ENH: add a module for pyplot like UX #3
Conversation
This works for me. Not sure which design you think is better to recommend as "canonical". import mpl_gui.registry as mgr
import numpy as np
fig, ax = mgr.subplots()
ax.plot(np.arange(10))
fig2, ax = mgr.subplots()
ax.plot(np.arange(10))
mgr.show_all() |
I think the trade off is if you want to have a very localized registry (because you want to be sure that you will have no conflicts with anyone else (either them breaking you or you breaking them)) but then if you want to share your registry between more than one modules, then you have to manage the global state your self. On the other hand using this module import (which effectively has a global singleton of Registry) you do not have to manage any of the state and there is an easy way to share between modules that may not know about each other but you open your self up to interference. I think both are valid use cases. I would like to do this as:
If (when) we move this upstream, I think this is the code that is going to have to be dropped into pyplot. |
932dcf5
to
3a2e196
Compare
mpl_gui/__init__.py
Outdated
@@ -198,7 +232,7 @@ def show_all(self, *, block=None, timeout=None): | |||
|
|||
if timeout is None: | |||
timeout = self._timeout | |||
|
|||
self._promote_and_number() |
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.
Why are figures only promoted on show_all? Maybe have a comment explaining this? I'm also not clear on the "promoted" terminology or why you need this concept. It would seem to me that all figures would just be added to the registry and numbered as they come in...
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.
How we got to this weird state:
- the goal is to create
Figure
objects without immediately creating theFigureManger
+FigureCanvas
because the later two are backend-dependent and in most (all?) cases create objects in the c/c++ layer - the notion of "number" lives on
FigureManagerBase
not inFigure
- thus deferred numbering the Figures until we are forced to actually create the
FigureManager
It would be reasonable to number the Figures
when they are created, but I was being lazy about creating extra state...
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.
and right after writing this I went back to read the code and they are already getting numbered on creation! re-working this...
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.
And now I remember the other complexity I was avoiding.
The counting we are doing at figure registration time is montonic figure count (because that is simple!) but was trying to reproduce the "restarting at 1" behavior of pyplot. I think the options here are:
- abandon the pyplot-like numbering and just accept manager monotonic increasing
- abandon the monotonic counting
In either case we need some way to associate the number atFigure
creation time with theFigure
. Options I can think of: - start to keep a weak-key dict mapping
Figure
-> number (I tried very hard to make there only be one place inFigureRegistry
that we keep a hard ref to theFigure
(inFigureRegistry.figures
, all of the other views are strictly derived from this). While we can not keep users from keeping hard-refs, we should be as sure as possible that we do not leak them - monkey patch
Figure
to carry the number
I do not think we should push the notion "number" into Figure
itself as there is an implication that the number is (temporarily) unique within a process/registry and part of the goal here is to pull apart Figure
, FigureManager
, and the Registry
.
I'm leaning towards monkey-patching for now, but changing the canonical version of the figure refernce to be a dictionary mapping int -> Figure
also seems like a good option.
Annoyingly, this PR is from before I moved the repo to the Matplotlib org so this branch is on upstream (not my fork). I'm going to force push to this and assume anyone who is currently relying on this branch knows what they are doing is can absorb a bit of chaos. |
bdc07de
to
b4f9dde
Compare
Can the docs be updated as well? Might make this easier for me to parse. I think this works well, but I didn't test all the blocking etc. For some of it, I would not object to a clean break with pyplot. It just seemed a bit mean to expect people to hoist a registry in place, when we could easily do it ourselves. OTOH, if it stops people from tripping over themselves using global state when they should not be using it (i.e. inside a discrete module), maybe we should make folks instantiate their own registry? We should get some opinions from other folks here (of your choosing). My interactive needs are pretty basic. I wonder about doing away with Sorry, more of an overview review than a code review ;-) |
@tacaswell where is this one at? |
@tacaswell sorting this out seems to be the last blocker on getting this in the main library at least on an experimental basis. |
5a4071b
to
bef9802
Compare
The thing I was hung up on (how to manage figure numbering "like pyplot") is sorted. |
- make passing num required from promote_figure - make _creation.figure function naive to interactive mode - switch from list to dict keyed on Figures - go with pyplot style numbering - improve the clean up logic to try and not leak refs Closes #4 Co-authored-by: Jody Klymak <[email protected]>
bef9802
to
aff3c3d
Compare
I'm going to self-merge this and then open a second PR for the docs. |
close #2
I only smoke-tested this, but seems to work.