JPG compression for inline pylab #4679

dbarbeau · 2013-12-12T13:10:34Z

This is a follow up to #4448

It adds JPG compression and quality options to render inline figures. This is useful for blogs where PNG isn't compact enough and SVG overly-verbose.

Carreau · 2013-12-15T18:27:16Z

This looks good to me.

I'll just re-ask the same question that I did when the retina key has appeard : what if you want jpeg double res for retina ?

Carreau · 2013-12-15T18:28:00Z

Would you like to drop a line in the what's new and the doc at the same time ?

dbarbeau · 2013-12-15T19:24:21Z

I don't know the history behind the retina key but just now, I'd say it doesn't fit well in the "figure_format" flag. It is some sort of extra-qualifier. Maybe a syntax like png:2x for the figure_format option could give more flexibility:

jpg:2x for JPG Retina
png:4x for PNG Super-Retina

The :2x part would be optional and if absent it would be synonym of :1x. The retina key would be kept as synonym of png:2x but deprecated. This also avoids adding an extra flag and preserves the current behaviour so users aren't lost.

Will look into docs and what's new.

dbarbeau · 2013-12-15T21:11:38Z

I added some lines in the what's new pr directory. I didn't find a convenient place to add extra doc and found the options page was autogenerated. Any tips to where I could put more information?

dbarbeau · 2013-12-15T21:19:04Z

Oh, I didn't implement the proposal above (png:2x syntax) because I think it belongs to another PR and I don't know the code well enough to be sure such a syntax wouldn't have side-effects in places expecting a simple format and receiving that sort of composite string.

takluyver · 2013-12-15T21:20:37Z

IPython/kernel/zmq/pylab/config.py

+
+    if has_pil:
+        # If we have PIL using jpeg as inline image format can save some bytes.
+        fmts.append('jpg')


I'm not convinced about conditionally including options for config - it could lead to unclear failures if someone sets the option to jpg and later removes PIL, because the error message will just say that jpg is not a valid value. It also means that we import PIL, which AFAIK is a relatively heavy library, even when it's not going to be used. I'd rather have jpg as an option all the time, and check for PIL when jpg is specified.

I agree about the weight of the import. However, I feel uncomfortable about advertising jpg and then, when it is about to be used, fail. It is an un-kept promise and a late failure. Documenting it in the help string can help, but won't prevent late failure during Notebook execution. What is the general policy?

If late failure is a concern, I think it should be possible to have a _figure_format_changed function which, if you select jpg, tries to import PIL, and warns you if it can't. But I'd rather the error message said "can't import PIL" or similar, rather than "jpg is not a valid option".

I don't think we have a general policy for this kind of thing.

damianavila · 2013-12-16T00:38:22Z

I suggest using Pillow instead of PIL... it is a fork of PIL but a lot of better and it will probably replace PIL after all...
If I remember correctly, you import Image in the same way... so you would need to changes only some references which I commented in the code above 😉

minrk · 2013-12-16T00:39:17Z

Just a note: -1 to @damianavila's proposed additions of pillow to comments / docstrings.

takluyver · 2013-12-16T01:01:32Z

I'd mention Pillow in docstrings, but possibly as "PIL/Pillow"

damianavila · 2013-12-16T01:04:38Z

Just a note: -1 to @damianavila's proposed additions of pillow to comments / docstrings.

and because ?

I'd mention Pillow in docstrings, but possibly as "PIL/Pillow"

I can live with this proposal...

dbarbeau · 2013-12-16T10:58:31Z

I think the PIL/Pillow import method is the same, and I'm actually using Pillow :) I can document it as PIL/Pillow, and be generic in the code by referring to it as "python_imaging". i really don't mind.

damianavila · 2013-12-16T11:46:09Z

I'm actually using Pillow :) I can document it as PIL/Pillow, and be generic in the code by referring to it as "python_imaging".

I think that mention of Pillow will save a lot of headaches to future users who don't know about this better fork of PIL... but it would be better to wait for @minrk comments about his -1 to make some of these changes, before actually doing the changes...

minrk · 2013-12-16T18:08:11Z

I think it's okay to mention just 'PIL' in the comments, since the import is the same, and we don't need to add clutter to every comment. You need the library, it doesn't matter how you install it. Mentioning Pillow in the what's new snippet is fine, though.

damianavila · 2013-12-17T00:18:40Z

I think it's okay to mention just 'PIL' in the comments, since the import is the same, and we don't need to add clutter to every comment. You need the library, it doesn't matter how you install it. Mentioning Pillow in the what's new snippet is fine, though.

OK, but I still would like to see a reference in the docstrings... maybe with the suggestion from @takluyver: "PIL/Pillow"
The people read what's new once, but the source is read it a lot of times... so aiding the user with just a few words in docstrings do not seem to much to add to me...

minrk · 2013-12-17T00:24:55Z

I just think adding PIL/Pillow to every instance is useless clutter. At most one location seems like plenty.

damianavila · 2013-12-17T00:35:10Z

At most one location seems like plenty.

Yes, but I do not think the what's new was the proper location... but, at the end, it is a little detail... and not a reason to stop the flow of this PR... ▶️ 😉

minrk · 2013-12-17T00:47:49Z

Sorry - by one location I meant in the code (not counting what's new). Mentioning it in the print_figure docstring and/or the traitlet help string is fine, but renaming variables as you proposed is a bit too far.

minrk · 2013-12-17T00:49:59Z

IPython/core/pylabtools.py

@@ -338,5 +342,5 @@ def configure_inline_support(shell, backend):
            del shell._saved_rcParams


This won't quite toggle correctly when moving from png to jpg, or jpg to anything - let's add [ f.type_printers.pop(Figure, None) for f in svg,png,jpg... ] to the top, then only turn them on in the if/elif/elif branch. That will be more consistent.

I must admit I don't totally understand what's going on there and I proceeded by copy/pasting. I see types are [un]registered so that formatters know what to handle and how to handle them. I don't get why SVG formatter was altered in each branch of the if/elif... If you can provide some background I'll be very grateful!

Thanks!

We only want one formatter registered at a time, otherwise every format will be published. This is saying "forget what we were publishing before, just publish the selected format." So, when png is selected, stop publishing svg and vice versa.

We only want one formatter registered at a time, otherwise every format will be published

I'll rebring this issue later, but I feel like we should also allow many formater no ?

damianavila · 2013-12-17T00:50:45Z

Mentioning it in the print_figure docstring and/or the traitlet help string is fine, but renaming variables as you proposed is a bit too far.

I agree that is maybe too far to rename variables... I will be happy with

in the print_figure docstring and/or the traitlet help string

Great we have achieved a consensus, hehe... 👍

minrk · 2013-12-17T00:51:26Z

IPython/kernel/zmq/pylab/config.py

@@ -53,7 +59,20 @@ def _config_changed(self, name, old, new):
        inline backend."""
    )

-    figure_format = CaselessStrEnum(['svg', 'png', 'retina'], default_value='png', config=True,
+    fmts = ['svg', 'png', 'retina']


add jpg to the formats, and then add a check for has_pil in _figure_format_changed, such as:

def _figure_format_changed(self, name, old, new): if new == 'jpg' and not has_pil: raise TraitError("Require PIL/Pillow for jpg figures")

And if we're doing this, may as well delay the import of PIL until jpg is set.

Ok, I think I can live with having the PIL import inside this function. Will do!

minrk · 2013-12-17T00:56:30Z

Actually, let's back up for a second - why do we check for PIL anyway? matplotlib jpeg output seems to work fine without PIL.

minrk · 2013-12-17T01:04:05Z

Nevermind - I was testing a non-agg backend. PIL is needed for jpg output with agg. Ignore me, I'm going home :).

damianavila · 2013-12-17T01:05:37Z

No problem, it is late here too ;-)

dbarbeau · 2013-12-17T18:04:29Z

Ok, I think the latest commit addresses most (if not all) of your comments. I tried to test it the best I could but I have a few concerns :

its quite hard to make ipython notebook print out warn() or logging.debug(...) or even print()! I had to attach another frontend to the kernel and call %debug to obtain feedback upon exceptions.
is calling nosetests IPython/kernel/zmq/tests the way to run tests locally?

As always any feedback is welcome!

dbarbeau · 2013-12-17T19:48:12Z

If you do %matplotlib inline, warn statements will be captured in the cell (I would recommend against ever starting the notebook with --matplotlib/pylab, I assume that's why you can't see your messages). logging.debug on the other hand, will only show up (in the terminal) if you have set the log-level to debug, e.g. with ipython notebook --debug

I was indeed doing notebook --pylab=inline and was worried because I didn't get any errors! I tried %pylab inline which seems to work and now I do get exceptions in my cells! Thanks, and thanks for the iptest info too.

Carreau · 2013-12-18T13:13:01Z

docs/source/whatsnew/pr/inline-jpg.rst

@@ -0,0 +1,2 @@
+* The InlineBackend.figure_format flag now supports JPEG output if PIL is available.
+* The new InlineBackend.quality flag controls the amount of compression (currently JPEG only)


InlineBackend.quality between backticks ?

Carreau · 2013-12-18T13:15:12Z

Looks good and test are passing. I upvote.

damianavila · 2013-12-18T16:34:47Z

Me too 👍

minrk · 2013-12-18T18:38:38Z

I'll rebring this issue later, but I feel like we should also allow many formater, no?

Yes, I think so, and it's not hard to actually do. Right now, you can:

for fmt, mime in [('svg', 'image/svg+xml'), ('png', 'image/png'), ('jpg', 'image/jpeg')]:
    f = ip.display_formatter.formatters[mime]
    f.for_type(Figure, lambda fig: print_figure(fig, fmt)

It's just a question of API, and the default behavior makes the most sense with only one format published. I think you proposed a 'formats' configurable, which would be a list rather than the current single string. But that's separate from this issue, which I agree is ready to go.

dbarbeau · 2013-12-20T14:19:00Z

Hello guys!

I was wondering if you'd like me to rebase this. I ask because I've read that rebasing can be dangerous, but since it hasn't been merged yet I don't think it should be an issue. However, I'd like to know your opinion!

Thanks

Carreau · 2013-12-20T14:37:10Z

It merge cleanly so there is no need to rebase for now. We usually rebase only when there is a merge conflict.

dbarbeau · 2013-12-20T16:32:21Z

OK!

I'm looking into adding unit tests but I don't really get how to do this for such a function. I'd like to create a code cell which creates a pylab figure and just test that the server returns a jpg if I asked it to. I don't see well how to proceed? Should start IPython notebook and talk HTTP directly to it?

takluyver · 2013-12-20T17:42:52Z

There's a test IPython.core.tests.test_pylabtools.test_figure_to_svg that you could base it on, and check the magic number for JPEG files. I think testing a separate process for this is probably overkill.

dbarbeau · 2013-12-20T22:44:52Z

Ah right, thanks I was looking in the kernel package, not the core package. Will further test my test and commit.

damianavila · 2013-12-29T14:09:48Z

IPython/core/tests/test_pylabtools.py

+try:
+    from PIL import Image
+    def test_figure_to_jpg():
+        # simple check for at least svg-looking output


svg-looking? I think is jpg 😉

minrk · 2014-01-24T23:27:24Z

Sorry, looks like this needs a rebase.

dbarbeau · 2014-01-25T11:56:08Z

Oh noooo! ^^ Will do!

…able.

… instead of testing it at module startup. The test is only run if figure_format is jpg. In help/error messages, refer to PIL/Pillow (i.e. not just PIL).

Saner acceptable range for jpeg quality parameter. Better docstrings.

ellisonbg · 2014-01-27T22:01:48Z

Looks like this has been rebased - is it ready for merge?

minrk · 2014-01-27T22:48:50Z

Yup. Thanks, @dbarbeau!

JPG compression for inline pylab

takluyver reviewed Dec 15, 2013
View reviewed changes

minrk reviewed Dec 17, 2013
View reviewed changes

Carreau reviewed Dec 18, 2013
View reviewed changes

damianavila reviewed Dec 29, 2013
View reviewed changes

dbarbeau added 9 commits January 25, 2014 15:16

Add JPEG as an image format for inline backend if PIL/pillow is avail…

1603a5a

…able.

update what's new

35b1975

Move testing for PIL[low] in the _figure_format_changed(...) function…

c954822

… instead of testing it at module startup. The test is only run if figure_format is jpg. In help/error messages, refer to PIL/Pillow (i.e. not just PIL).

Forgot to import TraitError.

f3fd178

Saner acceptable range for jpeg quality parameter. Better docstrings.

A somewhat better whatsnew

3bf72cd

add unittest for jpg, only activated if PIL/Pillow is installed

3b6c3ca

fix docstring for pylab jpg unittest

bb1ff13

use testing decorators to enable tests on module availability

56dda7c

add default value for select_figure_format's quality parameter

31c1d4f

minrk added a commit that referenced this pull request Jan 27, 2014

Merge pull request #4679 from dbarbeau/jpg-inline

6def8ea

JPG compression for inline pylab

minrk merged commit 6def8ea into ipython:master Jan 27, 2014

mattvonrocketstein pushed a commit to mattvonrocketstein/ipython that referenced this pull request Nov 3, 2014

Merge pull request ipython#4679 from dbarbeau/jpg-inline

5e8afca

JPG compression for inline pylab

		@@ -338,5 +342,5 @@ def configure_inline_support(shell, backend):
		del shell._saved_rcParams

		@@ -0,0 +1,2 @@
		* The InlineBackend.figure_format flag now supports JPEG output if PIL is available.
		* The new InlineBackend.quality flag controls the amount of compression (currently JPEG only)

JPG compression for inline pylab #4679

JPG compression for inline pylab #4679

Conversation

dbarbeau commented Dec 12, 2013

Carreau commented Dec 15, 2013

Carreau commented Dec 15, 2013

dbarbeau commented Dec 15, 2013

dbarbeau commented Dec 15, 2013

dbarbeau commented Dec 15, 2013

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

damianavila commented Dec 16, 2013

minrk commented Dec 16, 2013

takluyver commented Dec 16, 2013

damianavila commented Dec 16, 2013

dbarbeau commented Dec 16, 2013

damianavila commented Dec 16, 2013

minrk commented Dec 16, 2013

damianavila commented Dec 17, 2013

minrk commented Dec 17, 2013

damianavila commented Dec 17, 2013

minrk commented Dec 17, 2013

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

damianavila commented Dec 17, 2013

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

minrk commented Dec 17, 2013

minrk commented Dec 17, 2013

damianavila commented Dec 17, 2013

dbarbeau commented Dec 17, 2013

dbarbeau commented Dec 17, 2013

Choose a reason for hiding this comment

Carreau commented Dec 18, 2013

damianavila commented Dec 18, 2013

minrk commented Dec 18, 2013

dbarbeau commented Dec 20, 2013

Carreau commented Dec 20, 2013

dbarbeau commented Dec 20, 2013

takluyver commented Dec 20, 2013

dbarbeau commented Dec 20, 2013

Choose a reason for hiding this comment

Choose a reason for hiding this comment

minrk commented Jan 24, 2014

dbarbeau commented Jan 25, 2014

ellisonbg commented Jan 27, 2014

minrk commented Jan 27, 2014