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

Added IPythonViewer #5289

Merged
merged 5 commits into from Mar 7, 2021
Merged

Added IPythonViewer #5289

merged 5 commits into from Mar 7, 2021

Conversation

radarhere
Copy link
Member

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]

Copy link
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.



try:
from IPython.display import display
Copy link
Contributor

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
Member Author

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.

except ImportError:
pass
else:
register(IPythonViewer)
Copy link
Contributor

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
Member Author

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.

try:
viewer.get_command("test.jpg")
except NotImplementedError:
pass
Copy link
Contributor

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
Member Author

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?

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
Contributor

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
Member Author

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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Displaying images in Google Colab
4 participants