Skip to content

Added IPythonViewer#5289

Merged
hugovk merged 5 commits intopython-pillow:masterfrom
radarhere:ipythonviewer
Mar 7, 2021
Merged

Added IPythonViewer#5289
hugovk merged 5 commits intopython-pillow:masterfrom
radarhere:ipythonviewer

Conversation

@radarhere
Copy link
Copy Markdown
Member

@radarhere radarhere commented Feb 26, 2021

Resolves #5286 by adding a new ImageShow.Viewer - IPythonViewer. It calls the display() method to allow ImageShow to work with IPython... and Google Colab.

Here is my demonstration of it working in Google Colab.
Screen Shot 2021-02-26 at 11 09 38 pm

from PIL import Image, ImageShow
im = Image.new("RGB", (100, 100))
im.show()
from PIL import ImageShow
class IPythonViewer(ImageShow.Viewer):
    def show_image(self, image, **options):
        display(image)
        return 1

try:
    from IPython.display import display
except ImportError:
    pass
else:
    ImageShow.register(IPythonViewer)

from PIL import Image
im = Image.new("RGB", (100, 100))
im.show()

# Cleanup
ImageShow._viewers = [v for v in ImageShow._viewers if v.__class__ != IPythonViewer]

@radarhere radarhere mentioned this pull request Feb 26, 2021
Closed
@radarhere radarhere force-pushed the ipythonviewer branch 3 times, most recently from 5f1399c to 34b7edf Compare February 26, 2021 13:38
nulano
nulano approved these changes Feb 26, 2021
View reviewed changes
Copy link
Copy Markdown
Contributor

@nulano nulano left a comment

Choose a reason for hiding this comment

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

This should be very useful! Even if it takes a while until the changes reach Colab, it is not difficult to upgrade Pillow manually anyway.

src/PIL/ImageShow.py Outdated


try:
from IPython.display import display
Copy link
Copy Markdown
Contributor

@nulano nulano Feb 26, 2021

Choose a reason for hiding this comment

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

Suggested change
from IPython.display import display
from IPython.display import display as _display

Would it be reasonable to add a prefix to avoid confusion when using code completion tools? ImageShow.show and ImageShow.display are very similar and could be confusing to someone using it for the first time.

Copy link
Copy Markdown
Member Author

@radarhere radarhere Feb 26, 2021

Choose a reason for hiding this comment

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

Okay, sure. I went with ipython_display, to be even clearer.

src/PIL/ImageShow.py
except ImportError:
pass
else:
register(IPythonViewer)
Copy link
Copy Markdown
Contributor

@nulano nulano Feb 26, 2021

Choose a reason for hiding this comment

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

Any reason to put IPythonViewer before the unix viewers but after the other Windows/macOS ones? I think it would be best to put this at the top (or use register(..., order=0)) to help consistency in IPython (e.g. when combined with tools that offer PDF exports, etc.).

Copy link
Copy Markdown
Member Author

@radarhere radarhere Feb 27, 2021

Choose a reason for hiding this comment

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

Yep, no reason. I've moved it up.

Tests/test_imageshow.py
try:
viewer.get_command("test.jpg")
except NotImplementedError:
pass
Copy link
Copy Markdown
Contributor

@nulano nulano Feb 26, 2021

Choose a reason for hiding this comment

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

Suggested change
pass
assert viewer.show(hopper()) == 1

Would remove the need for a separate test.

Copy link
Copy Markdown
Member Author

@radarhere radarhere Feb 27, 2021

Choose a reason for hiding this comment

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

The separate test

  • confirms that the IPythonViewer is present when IPython is available
  • runs IPythonViewer even if another Viewer instance has priority

Also, IPython seems to be a special case, where running its Viewer doesn't disrupt the test process - thinking generically for the sake of the future, just because a Viewer isn't based on a command line instruction doesn't mean that we want it to run in the middle of the test suite?

This was referenced Feb 28, 2021
Merged
Closed
nulano
nulano reviewed Mar 3, 2021
View reviewed changes
docs/releasenotes/8.2.0.rst
registered. It displays images on all IPython frontends. This will be helpful
to users of Google Colab, allowing ``im.show()`` to display images.

It is lower in priority than the other default Viewer instances, so it will
Copy link
Copy Markdown
Contributor

@nulano nulano Mar 3, 2021

Choose a reason for hiding this comment

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

It is lower in priority than the other default Viewer instances

Please correct me if I'm wrong, but this is false. The new viewer has the highest priority AFAICT

Copy link
Copy Markdown
Member Author

@radarhere radarhere Mar 3, 2021

Choose a reason for hiding this comment

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

You're right! Thanks for catching that. However, the documentation correctly states my intention, so I've updated the code to match it.

@hugovk hugovk merged commit 6108596 into python-pillow:master Mar 7, 2021
@radarhere radarhere deleted the ipythonviewer branch March 7, 2021 21:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@nulano nulano nulano approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Displaying images in Google Colab

4 participants

@radarhere @nulano @hugovk @Kipkurui-mutai