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

use explicit axis, figure handles for plotting? #155

Open
bmaranville opened this issue Oct 28, 2022 · 6 comments
Open

use explicit axis, figure handles for plotting? #155

bmaranville opened this issue Oct 28, 2022 · 6 comments

Comments

@bmaranville
Copy link
Member

Right now we are using an implied context for plotting, calling pyplot.plot or pyplot.legend, or pyplot.ylabel etc. to make modifications to the currently "active" axes object or figure object. New subplots are being created on the currently active figure with e.g. plt.subplot(212)

These could all be replaced by explicitly defining figure with fig = plt.figure() then adding axes with e.g. axes = fig.add_subplot(212)
Then plotting can be called directly on the axes object with axes.plot(...)

This would require altering the plot routines to take an optional axis object (and figure object probably) that would be used in place of the currently active axis or figure. It would eliminate all the calls to gca() and gcf(), and could make the code easier to understand.

It would also make it safe to call plotting operations from a separate thread.

@bmaranville
Copy link
Member Author

It can be a non-breaking change if plotting routines all take optional axes=None and fig=None arguments, and then use gca() and gcf() if they are not supplied.

@pkienzle
Copy link
Member

pkienzle commented Nov 1, 2022

I'm not sure what the use case is, but consider using a context manager as an alternative:

with active_plot(fig=fig, axes=axes):
    do plotting

This can query the current figure and/or axes and set a new figure/axes on __enter__ and restore them on __exit__. Since some of the code uses pyplot.figure to create new figures you may want to redirect the figure manager as well.

A lot of the refl1d plots are coming from bumps so those would need to be updated. Worse is that the fig and axes parameters will need to be forwarded through all the intermediate functions. Bumps calls back to the problem definition to produce the plots, so any user-supplied FitProblem object with a custom show model method will need to accept these additional arguments.

@pkienzle
Copy link
Member

pkienzle commented Nov 1, 2022

BTW, I don't know if active_plot already exists or if it is something we need to write. I don't know if the %matplotlib [widget,interactive,notebook,...] magic will cause problems.

@bmaranville
Copy link
Member Author

Agreed that passing the fig, axes through intermediate layers is annoying, but it is more traceable. I went through and made most of the changes that would be needed for this in a test branch, and it wasn't all that much work.

We already have the "EmbeddedPylab" context which mostly does what active_plot would do (except when matplotlib changes their internal implementation functions, and then it breaks - it is relying on low-level code), but we don't need that if e.g. DataView holds on to its own figure object instead of trying to manage a context.

I am suggesting we'd be better off abandoning the old "context" paradigm for creating plots, whether it be implicit (as in most matplotlib examples) or explicit (with your new active_plot manager), because we can avoid a lot of headaches by just passing around fig and axes objects. Some reasons:

  1. it's safe to plot things in other threads (seems like having a thread for generating plots could be useful - this is how I got here)
  2. As Dave pointed out, you can't do certain things with the matplotlib context system, like get a handle to an inset plot with gca()
  3. It will be easier to maintain (less hidden variables)

@pkienzle
Copy link
Member

pkienzle commented Nov 2, 2022

I don't mind extra work in libraries (too much) but I hope we can shield users from extra complexity.

In bumps when fitting a custom model I'm asking users to provide a plotting function. I would prefer that they be able to use the pyplot interface to produce it since that's what web searching will give them. I don't know how to square this with your proposal, short of inspecting the plotting interface they supply to see if it will accept the figure/axes keywords we are passing around.

I'm still not sure how you deal with the call to figure() in bumps.dream.views.plot_all. I ended up with hard-coded calls to the individual plots for the separate tabs.

Do you need to pass both figure and axes handles? It's bad enough having to forward one nuisance parameter, nevermind two.

Maybe you solve both problems with a figure manager that has figure()/gcf()/gca() functions?

I might be overthinking this. You may just be asking that individual views use the axes that they are handed otherwise create one through pyplot, not something that solves the problem of automatically adding new tabs to the interface when dream creates a new kind of visualization. Your PR will make this clear enough 8-)

@pkienzle
Copy link
Member

pkienzle commented Nov 3, 2022

To be clear I consider Fitness.plot to be public interfaces that a user can implement for their own models. I guess we could add another method (Fitness.add_plot?) which takes a figure and/or axes. Then we can check if the new interface is available before calling:

...
if hasattr(self.fitness, "add_plot"):
    self.fitness.add_plot(fig, ax, view=view)
else:
    with pyplot_context(fig, ax):
        self.fitness.plot(view=view) 
...

I had intended FitProblem itself to be something that the user could provide. If we have a direct PDF implementation we should be able send it a numpy vector and returns an nllf with minimal overhead, and pass that to the various optimizers. This is helpful if you have large parameter vectors (e.g., every pixel is a separate fit parameter in an image). This code path has not been actively maintained, and I suspect additional requirements have slipped into the minimal FitProblem interface. The point is we should consider preserving backward compatibility for FitProblem.plot as well.

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

No branches or pull requests

2 participants