add context manager functionality to ion and ioff #17371

ianhi · 2020-05-09T03:06:10Z

PR Summary

Closes: #17013
Also closes: matplotlib/ipympl#220

This allows the usage of plt.ioff and plt.ion as context managers:

plt.ion()
with plt.ioff():
    # now interactive mode is off
    fig = plt.figure()
    ax = fig.gca()

plt.ioff()
with plt.ion():
    # now interactive mode is on
    fig = plt.figure()
    ax = fig.gca()

It also caches the state and returns to the original state on exit:

plt.ion()
with plt.ioff():
    # now interactive mode is off
    fig = plt.figure()
    ax = fig.gca()
assert plt.isinteractive

Documentation - WIP - advice welcome

I think that this additional behavior ought to be documented but I was a bit unsure how to go about it. In general things in the documentation seem divided into being functions or objects, and this PR puts ion and ioff somewhere between. So I think it makes sense to keep these on this page: https://matplotlib.org/3.2.1/api/_as_gen/matplotlib.pyplot.html?highlight=pyplot#module-matplotlib.pyplot. But, then they would be mislabelled as Functions, and I'm not sure that they will end up there anyway as that page is autogenerated. I also wasn't sure which docstring to put information about the context manager usage in, I think the most sensible place is in the __call__ method, but this isn't strictly what is used as the context manager.

The documentation needs be updated at these links:
https://matplotlib.org/3.2.1/users/shell.html#controlling-interactive-updating
https://matplotlib.org/3.2.1/tutorials/introductory/usage.html?highlight=usage#what-is-interactive-mode

I'll give this a go but wanted to get input on where to put extended docstrings and how to manage the autogenerating docs. Also, if anyone has any wording suggestions I'd be happy to hear them.

Tests

There don't seem to be any tests that make use of ion or ioff

cd lib/matplotlib/tests
grep -rni 'ioff()'

returns no results. Does this PR warrant adding new tests?

PR Checklist

Has Pytest style unit tests
Code is Flake 8 compliant
New features are documented, with examples if plot related
- See above discussion of documentation
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)
NA? Documented in doc/api/api_changes.rst if API changed in a backward-incompatible way

jklymak · 2020-07-23T16:06:26Z

@tacaswell @QuLogic @efiring I don't understand ion/ioff, but maybe this PR could get some discussion?

QuLogic · 2020-07-29T04:52:37Z

Seems fine, but it's still draft...

ianhi · 2020-07-29T04:55:19Z

I rebased and started trying to build the docs in order to see how my change affected the docs. I followed the instructions here (https://matplotlib.org/3.1.1/devel/documenting_mpl.html) and even tried them in a fresh conda env:

sudo apt install graphviz texlive texlive-xetex
conda create -n mpl-dev python
pip install -e .
pip install -r requirements/doc/doc-requirements.txt
cd docs
make html

But I keep running into this mystifying error:

Any advice for fixing that error/discovering what the error is? It makes it much harder to work out the above issues with the docs. Though I can confirm from the build artifact that the ioff doc page no longer exists.

ianhi · 2020-07-29T04:58:38Z

@QuLogic sorry I didn't see your comment before I posted the above. The code changes work for daily usage (I have started using this by monkeypatching it in and using daily) but the remaining issue is that by making ioff and ion instances of a class it changes how they show up in the docs and I am not sure if these changes are ok, or if not how to fix them.

this enables usage of plt.ioff as a context manager to allow creating figures without the figure automatically displaying in environments such as jupyter. Usage: with plt.ioff(): plt.figure()

need to ensure that the original state is restored post contextmanager

ianhi · 2020-08-05T16:27:22Z

I just rewrote this context manager in the same manner as the xkcd context manager so ion and ioff are functions once again so the docs to render correctly (still can't get them to build locally 😞 so pushing to see the docs build). I also added tests and an api note + new docs and will update the original post's checklist accordingly. If all the tests pass then I think this will be ready for review

ianhi · 2020-08-05T17:21:44Z

@jklymak I think this is ready for review. Could you undo the marking as draft (I don't seem to be able to)

New documentation links:
ioff
ion

jklymak · 2020-08-05T18:03:20Z

... The "Ready For Review" button doesn't work? Happy to click it for you but just wanted to see what the interface is?

ianhi · 2020-08-05T18:10:41Z

... The "Ready For Review" button doesn't work? Happy to click it for you but just wanted to see what the interface is?

🤦 I was looking at the top right section of the page - totally the wrong place. Sorry about that and thanks for the tip

jklymak · 2020-08-05T18:11:55Z

NP, just didn't want to convert things to draft if the author couldn't un-draft it.....

QuLogic

Does this work recursively?

with plt.ion():
    with plt.ion():
         pass

or

with plt.ion():
    with plt.ioff():
         pass

or even

with plt.ioff():
    with plt.ion():
         with plt.ioff():
            pass

doc/api/next_api_changes/behavior/17371-IHI.rst

lib/matplotlib/pyplot.py

ianhi · 2020-08-05T21:30:36Z

Does this work recursively?

Yes it does. Would you like me to add that to the tests?

It turns out that by using call rather than init I was breaking the normal usage of ion and ioff so I switched to using init and updated the tests so that they would catch that. One edge case here is: If you save the return value from a call to ioff or ion and try to use it as a contextmanager again it will no longer work.

This is pretty niche and hopefully no one is saving the result of plt.ioff but perhaps I should add a _used variable that is set to true on __exit__ and then raise a warning in __enter__? Like so:

class _ioff:
    _used = False

    def __init__(self):
        self.wasinteractive = isinteractive()
        matplotlib.interactive(False)
        uninstall_repl_displayhook()

    def __enter__(self):
        if self._used:
            cbook._warn_external("re-using an ioff contextmanager is",
                                 " unsupported and may have unexpected results")

    def __exit__(self, exc_type, exc_value, traceback):
        if self.wasinteractive:
            matplotlib.interactive(True)
            install_repl_displayhook()
        self._used = True

10000

ianhi · 2020-08-05T21:42:33Z

The tests all failed due to a primordial implementation of _used sneaking into the commit :/. I can fix that either by removing or implementing the code proposed in the above comment. So I'll wait for feedback re the warning

lib/matplotlib/pyplot.py

Co-Authored-By: Tim Hoffmann <timhoffm@users.noreply.github.com>

ianhi · 2020-08-06T00:43:19Z

I added more information to docstrings. Thinking more about the previously proposed _used approach I realized it can't be used to protect against using the return value as a contextmanager. What would be necessary is to detect if the __init__ method was called inside of a with context, but that doesn't seem possible. So this pathological case will remain possible:

An alternative to adding extra documentation and then hoping that users don't do this we could create a separate plt.ioff_context function. This would solve the above issue, but it would add cognitive load to the user. I at least find it more intuitive to call with plt.ioff(): than with plt.ioff_context():. If the tradeoff of not having ion/ioff have return values is deemed worthwhile then I'm happy to switch to that approach.

As an aside: What is difference between single and double backticks as used in matplotlib docstrings? This SO question seems to imply that the meaning of single backticks can vary project to project.

QuLogic · 2020-08-06T00:54:03Z

This is reStructuredText; single are cross-references, double are code blocks.

timhoffm · 2020-08-06T08:23:47Z

For cross-references see also https://matplotlib.org/devdocs/devel/documenting_mpl.html#referring-to-other-code.

Let me think a little about returning a context manager vs. a separate function.

timhoffm

IMHO it is better to have a function with a (potentially unused) return value context object than creating additional functions ioff/ion_context().

One strong argument is that standardlib open() also does not have two functions.

The suggestions try to make the situation still a little bit more clear.

lib/matplotlib/pyplot.py

ianhi · 2020-08-09T03:03:15Z

IMHO it is better to have a function with a (potentially unused) return value context object than creating additional functions ioff/ion_context().

@timhoffm my initial solution avoided the issue of the return value by making ion/ioff instances of a class that implemented __call__, __enter__, and __exit__. I've actually been using that myself in the meantime (ref/code example) but for the life of my I couldn't figure out how to make it work with the documentation as this made ion/ioff no longer functions. My initial post on this PR has details on this. But I think that the solution we've ended up at should be just fine :). It is the same approach taken by the xkcd context manager and by rc_context.

I've updated the documentation with your helpful clarifications, and I made the examples in ion and ioff have the same structure.

QuLogic

What about testing a nested context?

doc/api/next_api_changes/behavior/17371-IHI.rst

Co-authored-by: Elliott Sales de Andrade <quantum.analyst@gmail.com>

ianhi · 2020-08-11T04:34:12Z

I added variations on nested contexts all of which work. I did think of one more pathological case. What is the expected behavior for:

plt.ioff()
with plt.ioff():
    plt.ion()
print(mpl.is_interactive())

currently the context manager will leave you in interactive mode at the end

It should be easy to change it so that interactive mode is off in the end, but I'm 50/50 on what's best.

dopplershift · 2020-08-11T05:19:39Z

I think the context manager should change state on entry into the block, and restore the previous state on exit. Therefore IMO at the end of your code interactive mode should be off.

jklymak marked this pull request as draft July 23, 2020 16:05

jklymak added status: needs rebase status: needs comment/discussion needs consensus on next step labels Jul 23, 2020

ianhi force-pushed the context-manager branch from e1aa03e to 8c3e60c Compare July 29, 2020 04:21

ianhi added 3 commits August 5, 2020 12:14

add context manager functionality to ion and ioff

2458155

this enables usage of plt.ioff as a context manager to allow creating figures without the figure automatically displaying in environments such as jupyter. Usage: with plt.ioff(): plt.figure()

add ion/ioff context manager tests

7277226

need to ensure that the original state is restored post contextmanager

add ioff/ion context api note

0f5840c

ianhi force-pushed the context-manager branch from 8c3e60c to 0f5840c Compare August 5, 2020 16:22

test interactive mode context managers correctly

80161d0

ianhi marked this pull request as ready for review August 5, 2020 18:10

QuLogic reviewed Aug 5, 2020

View reviewed changes

doc/api/next_api_changes/behavior/17371-IHI.rst Outdated Show resolved Hide resolved

lib/matplotlib/pyplot.py Outdated Show resolved Hide resolved

lib/matplotlib/pyplot.py Outdated Show resolved Hide resolved

restore existing ion/ioff behavior + add test

0d93f1b

timhoffm reviewed Aug 5, 2020

View reviewed changes

lib/matplotlib/pyplot.py Outdated Show resolved Hide resolved

lib/matplotlib/pyplot.py Outdated Show resolved Hide resolved

timhoffm reviewed Aug 5, 2020

View reviewed changes

lib/matplotlib/pyplot.py Show resolved Hide resolved

improve ioff/ion docstrings

afe8d07

Co-Authored-By: Tim Hoffmann <timhoffm@users.noreply.github.com>

timhoffm reviewed Aug 8, 2020

View reviewed changes

lib/matplotlib/pyplot.py Outdated Show resolved Hide resolved

lib/matplotlib/pyplot.py Outdated Show resolved Hide resolved

lib/matplotlib/pyplot.py Outdated Show resolved Hide resolved

lib/matplotlib/pyplot.py Outdated Show resolved Hide resolved

add documentation warning against using return value of ion/ioff

ea74eff

ianhi changed the title ~~WIP: add context manager functionality to ion and ioff~~ add context manager functionality to ion and ioff Aug 9, 2020

QuLogic reviewed Aug 11, 2020

View reviewed changes

doc/api/next_api_changes/behavior/17371-IHI.rst Outdated Show resolved Hide resolved

doc/api/next_api_changes/behavior/17371-IHI.rst Outdated Show resolved Hide resolved

ianhi and others added 2 commits August 11, 2020 00:19

grammar and formatting of api note

0b11f80

Co-authored-by: Elliott Sales de Andrade <quantum.analyst@gmail.com>

test nested ion/ioff contexts

7746cdf

ianhi added 2 commits August 11, 2020 21:15

ensure final state is same as initial for ion/ioff contexts

d478070

correctly link to ioff/ion

a7eb704

QuLogic removed the status: needs rebase label Aug 12, 2020

QuLogic approved these changes Aug 12, 2020

View reviewed changes

dopplershift approved these changes Aug 12, 2020

View reviewed changes

dopplershift added this to the v3.4.0 milestone Aug 12, 2020

timhoffm approved these changes Aug 12, 2020

View reviewed changes

timhoffm merged commit 89d13c3 into matplotlib:master Aug 12, 2020

ianhi deleted the context-manager branch August 12, 2020 14:06

QuLogic removed the status: needs comment/discussion needs consensus on next step label Mar 17, 2021

Uh oh!

add context manager functionality to ion and ioff #17371

add context manager functionality to ion and ioff #17371

Uh oh!

Conversation

Uh oh!

PR Summary

Documentation - WIP - advice welcome

Tests

PR Checklist

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!