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

Fixes to `.plot()` (for Map) and `.peek()` (for all classes) #3103

Merged
merged 11 commits into from May 22, 2019

Conversation

Projects
5 participants
@ayshih
Copy link
Contributor

commented May 10, 2019

This PR:

  • Removes the toggle_pylab decorator everywhere because it led to wonky behavior in Jupyter notebooks when using the inline backend, with figures sometimes auto-showing and sometimes not auto-showing depending on the sequence of calls
  • Changes peek() so that it calls figure.show() only if an interactive Matplotlib backend is being used
  • Changes all peek() methods to use plt.show() instead of Figure.show(), and plt.show() is called regardless of which Matplotlib backend is being used. This change means that peek() methods now respect Matplotlib's blocking/non-blocking behavior (i.e., whether interactive mode is on or off)
  • Uses a new implementation for peek() methods using a decorator (peek_show) so that we can easily modify the behavior of peek() methods in the future as needed (e.g., when Matplotlib enhances plt.show() to be able to show only one figure)

Fixes #3094 and closes #2352

@sunpy-bot

This comment has been minimized.

Copy link

commented May 10, 2019

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

@ayshih

This comment has been minimized.

Copy link
Contributor Author

commented May 10, 2019

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.

@ayshih ayshih changed the title Fixes to Map's plot() and peek() Fixes to `.plot()` (for Map) and `.peek()` (for all classes) May 10, 2019

@ayshih

This comment has been minimized.

Copy link
Contributor Author

commented May 10, 2019

Not just for Map anymore!

@Cadair Cadair added this to the 1.0 milestone May 14, 2019

@Cadair Cadair added this to Post-Feature Freeze in SunPy 1.0 May 14, 2019

@Cadair Cadair added the [BugFix] label May 15, 2019

@Cadair Cadair modified the milestones: 1.0, 0.9.8 May 15, 2019

@Cadair Cadair dismissed their stale review May 15, 2019

Changed my mind.

@Cadair

This comment has been minimized.

Copy link
Member

commented May 15, 2019

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.

@dstansby
Copy link
Contributor

left a comment

In general, figure.show() can't be relied upon to properly show a figure; there is a warning in the latest version of the Matplotlib docs which isn't yet released: https://matplotlib.org/devdocs/api/_as_gen/matplotlib.figure.Figure.html?highlight=figure%20show#matplotlib.figure.Figure.show

I think the calls to figure.show() should either be changed to plt.show(), or be removed completely. I think the 'pythonic' way of doing things would be to have .peek() create and return a figure and Axes, and then let the user control showing of the figure, but guess the idea of peek() is to show the figure immediately.

@Cadair Cadair modified the milestones: 0.9.8, 0.9.9 May 15, 2019

@Cadair

This comment has been minimized.

Copy link
Member

commented May 15, 2019

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()?

@Cadair

This comment has been minimized.

Copy link
Member

commented May 15, 2019

@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.

@Cadair Cadair force-pushed the ayshih:peek branch from 441c22e to ee33a06 May 15, 2019

@Cadair Cadair requested review from dstansby and Cadair and removed request for dstansby May 15, 2019

@dstansby

This comment has been minimized.

Copy link
Contributor

commented May 15, 2019

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?

@ayshih

This comment has been minimized.

Copy link
Contributor Author

commented May 15, 2019

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).

@ayshih

This comment has been minimized.

Copy link
Contributor Author

commented May 15, 2019

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().

@ayshih

This comment has been minimized.

Copy link
Contributor Author

commented May 15, 2019

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.

@Cadair Cadair modified the milestones: 0.9.9, 1.0 May 16, 2019

@Cadair

This comment has been minimized.

Copy link
Member

commented May 16, 2019

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

@ayshih

This comment has been minimized.

Copy link
Contributor Author

commented May 16, 2019

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?

@ayshih

This comment has been minimized.

Copy link
Contributor Author

commented May 16, 2019

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

@dstansby

This comment has been minimized.

Copy link
Contributor

commented May 17, 2019

That sounds good to me.

Show resolved Hide resolved changelog/3103.bugfix.rst Outdated

nabobalis and others added some commits May 17, 2019

@Cadair

This comment has been minimized.

Copy link
Member

commented May 21, 2019

Are we happy with this as it is?

@ayshih

This comment has been minimized.

Copy link
Contributor Author

commented May 21, 2019

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. =)

@Cadair

This comment has been minimized.

Copy link
Member

commented May 22, 2019

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

@ayshih ayshih force-pushed the ayshih:peek branch from fa61a8e to a15a965 May 22, 2019

@ayshih ayshih force-pushed the ayshih:peek branch from a15a965 to a2104a4 May 22, 2019

@ayshih

This comment has been minimized.

Copy link
Contributor Author

commented May 22, 2019

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.

@Cadair

Cadair approved these changes May 22, 2019

@Cadair Cadair merged commit f45989a into sunpy:master May 22, 2019

16 checks passed

ci/circleci: 32bit Your tests passed on CircleCI!
Details
ci/circleci: egg-info-36 Your tests passed on CircleCI!
Details
ci/circleci: egg-info-37 Your tests passed on CircleCI!
Details
ci/circleci: figure-tests-36 Your tests passed on CircleCI!
Details
ci/circleci: html-docs Your tests passed on CircleCI!
Details
ci/circleci: pip-install Your tests passed on CircleCI!
Details
codecov/patch 96.15% of diff hit (target 90.07%)
Details
codecov/project 90.15% (+0.07%) compared to 80c2e97
Details
giles Click details to preview the documentation build
Details
sunpy-bot All checks passed
sunpy.sunpy Build #20190522.15 succeeded
Details
sunpy.sunpy (Linux_36_Conda_offline) Linux_36_Conda_offline succeeded
Details
sunpy.sunpy (Linux_36_offline) Linux_36_offline succeeded
Details
sunpy.sunpy (Linux_37_online) Linux_37_online succeeded
Details
sunpy.sunpy (Windows_36_offline) Windows_36_offline succeeded
Details
sunpy.sunpy (macOS_37_offline) macOS_37_offline succeeded
Details
@Cadair

This comment has been minimized.

Copy link
Member

commented May 22, 2019

@nabobalis backport please.

@Cadair Cadair moved this from Post-Feature Freeze to Finished in SunPy 1.0 May 22, 2019

nabobalis added a commit to nabobalis/sunpy that referenced this pull request May 22, 2019

Merge pull request sunpy#3103 from ayshih/peek
Fixes to `.plot()` (for Map) and `.peek()` (for all classes)

@ayshih ayshih deleted the ayshih:peek branch May 22, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.