Skip to content

Dedupe implementations of configure_subplots(). #16818

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

Merged
merged 1 commit into from
Aug 18, 2020

Conversation

anntzer
Copy link
Contributor

@anntzer anntzer commented Mar 18, 2020

Rely on pyplot auto-backend-detection to pop up a window with the
correct canvas class (technically this means one can end up with
a gtk3agg subplot tool when the main figure is gtk3cairo, but that
shouldn't be a real problem...).

  • Qt is excluded because it has its own (native) subplot config tool.
  • wx needs to restore a call to Destroy() because the subplot config
    figure is not pyplot-managed, so Gcf.destroy(self.num) is a noop.
  • Add a reference to the subplot config figure from the subplot tool
    widget.

PR Summary

PR Checklist

  • Has Pytest style unit tests
  • Code is Flake 8 compliant
  • New features are documented, with examples if plot related
  • Documentation is sphinx and numpydoc compliant
  • Added an entry to doc/users/next_whats_new/ if major new feature (follow instructions in README.rst there)
  • Documented in doc/api/api_changes.rst if API changed in a backward-incompatible way

@QuLogic
Copy link
Member

QuLogic commented Apr 22, 2020

This causes some kind of mainloop breakage, if you call plt.subplot_tool() as the first thing before plt.show(). Only one "Figure 1" appears, not the configuration tool, and closing it deadlocks the prompt.

@anntzer
Copy link
Contributor Author

anntzer commented Apr 22, 2020

Should be fixed now -- basically, because the toolfig is not pyplot-managed, I needed to call show() on it explicitly.

@QuLogic
Copy link
Member

QuLogic commented Apr 23, 2020

I guess it's still a bit weird, because it starts the mainloop now, which shows the figure immediately instead of on plt.show() (if you haven't called plt.ion()).

Actually, it doesn't quite act like plt.ion(), as regular figures do not open automatically after, but also plt.show() blocks until the subplot tool is closed, even though it opens without it.

@timhoffm
Copy link
Member

timhoffm commented Apr 23, 2020

General comment/opinion:

I'm not fond of trying to fake dialogs using figures. The main motivation is that we do not need to maintain dialogs for each backend.

But:

  • Figures are not dialogs, which causes awkward intercations (see comment above)
  • IMHO the look&feel of the matplotlib widgets is poor.

I'd prefer native dialogs and for that:

  • Define a narrowly limited scope of what we want to make configurable.
  • Implement this with native widgets for each backend.

This is code that will rarely change and once we have done the one-time effort of implementing it, it's not significantly more maintainance burden than the mpl widgets approach.

@anntzer
Copy link
Contributor Author

anntzer commented Apr 23, 2020

@QuLogic Can you clarify what is the failing case? TBH I always have a bit of trouble with following ion()/ioff()...

@timhoffm Just to be clear, I have a strong preference for native widgets too (after all I wrote #8683 :-)). However right now the matplotlib figure "is what we have", and this PR doesn't make things worse in that respect.

@timhoffm
Copy link
Member

@anntzer Correct, that's why I wrote "General comment".

Concerning this PR, I cannot easily review the change because I don't follow the details of figure creation. Would need to dig into it a bit further.

@QuLogic
Copy link
Member

QuLogic commented Apr 23, 2020

Well, it's not really ion mode, just that it sort of half-emulates it.

Previously, if you ran plt.subplot_tool(); plt.show(), then the tool would implicitly create a figure to work on, and both would show up when plt.show() was called. plt.show() would not exit until both were closed.

With this PR, it's 'half-interactive', i.e., the tool shows itself immediately, but the figure only shows on plt.show(). Then inconsistently, the plt.show() does not exit until both are closed.

Granted, it is a bit of an edge case to call subplot_tool first with nothing open, so maybe this is a tradeoff we'd be willing to make.

@anntzer
Copy link
Contributor Author

anntzer commented Apr 23, 2020

I'm fairly tempted to claim that indeed opening a subplot_tool without having a figure first is simply not something we really need to support. (I don't think there's a simple way to register somewhere that a non-pyplot window needs to be show()ed when pyplot.show() is called.)

manager.set_window_title("Subplot configuration tool")
tool_fig = manager.canvas.figure
tool_fig.subplots_adjust(top=0.9)
manager.show()
Copy link
Member

Choose a reason for hiding this comment

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

The work around there is to put the show on a 0s timer on the event loop. That will make it show as soon as the event loop is allowed to run again (which is either on return of control to the user or when plt.show() is called).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

do you want to put in that patch? given that you seem to know what to do...

@tacaswell
Copy link
Member

I have a slight preference for using the zero time timeout to defer calling show until after the event loop gets a chance to spin. In addition to just being a funny edge case, it makes it more likely that someone could do

def my_function():
    plt.configure_subplots()
    time.sleep(32) # or actual work

and be left with a "frozen" window.

However, if there is consensus that this a weird edge case I'm not going to block merging over this.

@tacaswell tacaswell modified the milestones: v3.3.0, v3.4.0 Jun 9, 2020
@tacaswell
Copy link
Member

Pushing to 3.4 as I don't think this is urgent.

@anntzer anntzer changed the title Dedupe implementations of configure_subplot(). Dedupe implementations of configure_subplots(). Jun 20, 2020
Rely on pyplot auto-backend-detection to pop up a window with the
correct canvas class (technically this means one can end up with
a gtk3agg subplot tool when the main figure is gtk3cairo, but that
shouldn't be a real problem...).

- Qt is excluded because it has its own (native) subplot config tool.
- wx needs to restore a call to Destroy() because the subplot config
  figure is not pyplot-managed, so `Gcf.destroy(self.num)` is a noop.
- Add a reference to the subplot config figure from the subplot tool
  widget.
@anntzer
Copy link
Contributor Author

anntzer commented Jul 20, 2020

This may be nice to get in at least before the big MEP22 work.

Copy link
Member

@tacaswell tacaswell left a comment

Choose a reason for hiding this comment

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

We should in the future we should add a hook on the Figure / manager to spawn a new window of the same type (maybe not registered with pyplot?), but this OK to go in for now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants