Thanks for the pull request @ayshih! Everything looks great!

To be clear, toggle_pylab also causes weird behavior with how plot() calls are handled – i.e., it wasn't affecting just peek() – but no one noticed because it's not unusual to explicitly call plt.show() after the plot() call.

Not just for Map anymore!

So I think I am pretty much fine with this, but I think we could simplify things by making a decorator which does this and is solely used for peek.

The following decorator would maintain the current behaviour of peek() (which I suggest we implement in this PR so we can backport this)

def peek_show_interactive(func):
    """
    A decorator to place on ``peek()`` methods to show the figure.
    """
    @wraps(func)
    def show_if_interactive(*args, **kwargs):
        figure = func(*args, **kwargs)
        # Show the figure if using an interactive Matplotlib backend
        if mpl.get_backend() in mpl.rcsetup.interactive_bk:
            figure.show()
        # NOTE: We do not return `figure` here because `peek()` methods return `None`.

    return show_if_interactive

We could then optionally later on make the decorator return the figure object if we wanted to go back to having peek() return the figure. (I am really not sure on the pros and cons of that so we should discuss it separately), but it means changing the return behaviour of all peek() methods would be done in one place.

Oh goodness my brain.

We stopped peek() returning the figure object in #2487. I think this was because it was causing double plotting problems in the notebook with inline.

@dstansby Yeah we have always wanted the behaviour of peek() to be to show the plot immediately and for plot to be the one you customise.

Could we just replace the figure.show in this PR with plt.show()?

@ayshih I have added one commit on the end here which migrates this logic into a decorator. If you don't like it we can always remove it.

After further conversations with @dstansby I think we might want to tweak the logic later, so having it all in once place makes that easier.

👍 on the decorator approach

👎 on using plt.show() instead of Figure.show()

• This kills the use case where the user wants to see only the Map object, and not necessarily display any other pending figures that haven't yet been shown.
• If Figure.show(), and hence peek(), may not work properly in a pure Python shell, I'm okay with that (but it should be documented).
• In the case of scripts, I don't think we should be calling any .show() anyway (see https://matplotlib.org/faq/howto_faq.html#use-show), so it's irrelevant if Figure.show() may not work properly there.

👎 on removing the check for an interactive backend

• In the case of scripts, we shouldn't be calling .show() (see above).
• In the case of a custom interactive backend, the "obvious" solution is that the user of the custom backend should append that interactive backend to the Matplotlib list so that the check works.

I definitely don't think using Figure.show() is the right thing to do; "Proper use cases for Figure.show include running this from a GUI application or an IPython shell.", and neither of these is what .peek() is trying to do.

Sadly I think using plt.show() is unavoidable. There is a PR to add the ability to do plt.show([fig1]) to only show fig1 to Matplotlib (matplotlib/matplotlib#14024), so eventually this will not be a problem.

I'm not sure what the problem is with calling .show() from a script? I do it regularly to show a figure, check it, and then close it to leave the script running from where it left off. Can't a user always just do map.plot() if they don't want to show it immediately?

I definitely don't think using Figure.show() is the right thing to do; "Proper use cases for Figure.show include running this from a GUI application or an IPython shell.", and neither of these is what .peek() is trying to do.

I don't know what you are saying here. What do you think that peek() is trying to do? I typically use peek() in two situations:

• In an IPython shell, so Figure.show() is explicitly appropriate
• In a Jupyter notebook, where no .show() is necessary, so we don't even need to argue whether to use Figure.show() or plt.show()

I don't use peek() in scripts, but perhaps that use case is more prevalent than I realize.

I'm not sure what the problem is with calling .show() from a script?

I don't actually know either, but I'd prefer to avoid supporting a use pattern that Matplotlib discourages and says is "unsupported" (assuming that their FAQ reflects their current conception).

I just did some testing, and Figure.show() appears to empirically work fine for us in pure Python shells (perhaps because of stuff that happens when SunPy is imported?). Even so, I'm giving up on Figure.show() because it doesn't appear to block in the manner of plt.show() (depending on whether in interactive mode). I don't think that peek() should block differently than plt.show().

Ha ha, I just stumbled across a comment of mine from just over five years ago:

Reasonable people can debate whether figure.show() or plt.show() is the correct call to make when displaying only a single figure, but figure.show() should work (and avoids the blocking behavior of plt.show()).

I guess back then I thought that peek() should never block.

The changelog on this needs updating as the behaviour has now changed to plt.show() over figure.show()

Following all of the discussion on peek(), I propose the following changes to complete this PR:

• Restore the check for a GUI backend before calling plt.show() so that it is not called when peek() is called in a script. (Even though Jupyter notebook's backends aren't GUI backends, plots will still display automatically there.) For script use, the user can choose to explicitly call plt.show() after peek() if that's what they want to happen, but they could also defer plt.show() until after multiple peek() calls, or they can save the figure (accessing it via plt.gcf()) for post-script evaluation.
• Use variations of the following text for all of the peek() docstrings:

Displays a graphical overview of the data in this object for evaluation purposes.
For the creation of plots, users should instead use the `~sunpy.map.GenericMap.plot()`
method and Matplotlib's pyplot framework.

This method is primarily intended to be used as part of an interactive workflow.
If this method is called from a script, the graphical overview will not display automatically;
instead, it is left to the user to explicitly call the appropriate display function
(e.g., `matplotlib.pyplot.show()`) at the point in the script that such display is desired.

Thoughts?

@Cadair pointed out that my "script" use case may not actually exist, so I need to find out whether I am crazy or not

That sounds good to me.

Are we happy with this as it is?

Mostly. I'm fairly convinced that the "script" use case that I was worried about earlier doesn't actually exist. However, I'd like the following changes:

• Change the name of the function inside the peek_show decorator to be something other than show_if_interactive(), since it no longer checks for a GUI backend (i.e., it always tries to show)
• Inside the decorator, trash the Figure object that is returned rather than assigning it to a variable (i.e., use the underscore) since that would represent the intent and is better for code analysis (since the variable is not subsequently used)
• Use variations of the following text within all of the peek() docstrings:

Displays a graphical overview of the data in this object for evaluation purposes.
For the creation of plots, users should instead use the `~sunpy.map.GenericMap.plot()`
method and Matplotlib's pyplot framework.

@Cadair Should I make these changes myself? You have more commits to this PR than I do. =)

@ayshih please always push things 😄 Whoever get's a chance first can do it!

Okay, I've made the changes that I wanted. I didn't update the peek() docstrings for the derived TimeSeries classes because their implementations needs to be cleaned up (i.e., source-specific plotting code should be accessible through a plot() method, but is only found in the peek() method). I think such changes should be put off to a different PR.

@nabobalis backport please.