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

Adding PDFFormatter and kernel side handling of PDF display data #4920

Merged
merged 9 commits into from Feb 8, 2014

Conversation

ellisonbg
Copy link
Member

This is a continuation of #4130 to add PDF handling.

@ellisonbg ellisonbg mentioned this pull request Jan 29, 2014
5 tasks
@ellisonbg
Copy link
Member Author

OK the basics are now working, along with a refactored InlineBackend. But @takluyver and @minrk there is a problem with the %config magic that prevents the following from working:

%config InlineBackend.figure_formats = {'png','pdf'}

The problem is that the IPython input machinery sees the {} syntax as string fomatting and removes it leading to the expression 'png','pdf' which is a tuple. We have two options here:

  1. Just make the figure_formats a List trait rather than Set.
  2. Dig in a try to find out why the {} is getting removed.

@ellisonbg
Copy link
Member Author

OK I have a simple OutputArea.append_pdf working. Here are screenshots of what it looks like:

screen shot 2014-01-28 at 11 41 03 pm

And with just PDF:

screen shot 2014-01-28 at 11 41 11 pm

@ellisonbg
Copy link
Member Author

Looks like we have another bug: display(gcf()) doesn't work any longer.

@ellisonbg
Copy link
Member Author

I have also tested this with nbconvert and it uses the embedded PDF files for latex/PDF output. Yeah!

@Carreau
Copy link
Member

Carreau commented Jan 29, 2014

Note that this would partially conflict with one PR of mine. Also I think at some point we were wondering if we shoudln't have a @interpolate(False|True) for magics to expand {} or not.

Now go to sleep Brian :-)

@jakobgager
Copy link
Contributor

Great!
If available, one could use Wand to create a thumbnail of the pdf to be displayed instead of, or in addition to the link, e.g. see http://stackoverflow.com/q/19470099/2870069

@minrk
Copy link
Member

minrk commented Jan 29, 2014

Disabling var_expand on several magics that don't actually want the extra magic would solve a lot of issues, but it's probably a task for another time.

In general, I'm not convinced that runtime config should really be the recommended approach for anything we expect people to actually change, since many configurables only make sense on first load, and can have weird, incomplete effects when changed at runtime. I would suggest making the recommended approach directly calling select_figure_format, and making that function available in an official public API.

@@ -79,6 +79,12 @@ def _figure_format_changed(self, name, old, new):
else:
select_figure_format(self.shell, new)

figure_format = Unicode()
Copy link
Member

Choose a reason for hiding this comment

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

This should still be configurable with a deprecation note

@ellisonbg
Copy link
Member Author

The decision from the dev meeting is to:

  • Not solve the bug with how IPython magics handle {}.
  • Keep the config stuff as is.
  • Offer a new public function to setting the figure format.

Commits on the way...

@ellisonbg
Copy link
Member Author

I just pushed a commit that adds this API:

IPython.display.select_matplotlib_formats(formats, quality=90)

Alternatives:

IPython.display.select_matplotlib_figure_formats(formats, quality=90)
IPython.display.set_mpl_figure_formats(formats, quality=90)

Also, much nicer to call most of the time is the *formats, **kwargs variation:

IPython.display.select_matplotlib_formats(*formats, **kwargs)

Thoughts?

@ellisonbg ellisonbg added this to the 2.0 milestone Feb 5, 2014
@ellisonbg ellisonbg self-assigned this Feb 5, 2014
@ellisonbg
Copy link
Member Author

We decided set_matplotlib_formats(*formats, **kwargs) with a docstring telling people about quality=90.

@ellisonbg
Copy link
Member Author

I have made the API changes discussed. For consistency I have also added a set_matplotlib_close function to IPython.display for interactive usage. I have also updated the docstrings of the various places that refer to this logic. I did rebase so we should wait for Travis to pass on this one.


In [1]: set_matplotlib_formats('png', 'jpeg', quality=90)

To set this in your config files use the following::
Copy link
Member

Choose a reason for hiding this comment

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

For equivalence with the example above, this should also indicate how to set the quality parameter in the config file.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

@fperez
Copy link
Member

fperez commented Feb 7, 2014

Looks great. I made some small comments about docs, and it's also missing an entry in the what's new/api changes documentation sources indicating the new/changed API calls.

Once these small fixes are in, this looks good to merge to me.

@ellisonbg
Copy link
Member Author

Review comments addressed, ready for merge.

ellisonbg added a commit that referenced this pull request Feb 8, 2014
Adding PDFFormatter and kernel side handling of PDF display data
@ellisonbg ellisonbg merged commit e68e13c into ipython:master Feb 8, 2014
@ellisonbg ellisonbg deleted the pdf-formatter branch February 8, 2014 19:49
@westurner
Copy link

@ellisonbg Two questions:

  1. Should this be listed as a choice with ipython nbconvert --help
    And/or: https://github.com/ipython/ipython/blob/master/examples/widgets/Export%20As%20(nbconvert).ipynb ?
  2. How does this change http://ipython.org/ipython-doc/dev/interactive/nbconvert.html ?

@westurner
Copy link

.3. Will there be a 'Download As' > 'PDF (.pdf)`?

Trying to respond to this: http://www.reddit.com/r/IPython/comments/1xzjrx/ipython_founder_details_road_map_for_interactive/cfg1ex2

?

@minrk
Copy link
Member

minrk commented Feb 15, 2014

Will there be a 'Download As' > 'PDF (.pdf)`?

Yes, but not immediately. We plan to replace the --post PDF with a PDFExporter, which does basically the same thing. This is a prerequisite for download-as-pdf.

@minrk
Copy link
Member

minrk commented Feb 15, 2014

Should this be listed as a choice with ipython nbconvert --help

This affects execution, it does not affect the choices in nbconvert.

How does this change http://ipython.org/ipython-doc/dev/interactive/nbconvert.html

Not at all.

@westurner
Copy link

Cool; thanks. So will that require all 1G of texlive-full or all
1.2G of MacTeX?

rst2pdf/xhtml2pdf (reportlab) may also work.

On 2/15/14, Min RK notifications@github.com wrote:

Will there be a 'Download As' > 'PDF (.pdf)`?

Yes, but not immediately. We plan to replace the --post PDF with a
PDFExporter, which does basically the same thing. This is a prerequisite for
download-as-pdf.


Reply to this email directly or view it on GitHub:
#4920 (comment)

Wes Turner

@minrk
Copy link
Member

minrk commented Feb 15, 2014

Cool; thanks. So will that require all 1G of texlive-full or all 1.2G of MacTeX?

You can get by with basictex and then installing the missing packages with tlmgr, but I don't recall what they are off the top of my head.

@ellisonbg
Copy link
Member Author

I should mention that with this change, if you have PDF output for a cell
it will automatically be used when you convert to LaTeX/PDF. This means no
more bitmapped matplotlib figures in nbconvert PDFs!

On Sat, Feb 15, 2014 at 11:43 AM, Min RK notifications@github.com wrote:

Cool; thanks. So will that require all 1G of texlive-full or all 1.2G of
MacTeX?

You can get by with basictex and then installing the missing packages with
tlmgr, but I don't recall what they are off the top of my head.

Reply to this email directly or view it on GitHubhttps://github.com//pull/4920#issuecomment-35165687
.

Brian E. Granger
Cal Poly State University, San Luis Obispo
bgranger@calpoly.edu and ellisonbg@gmail.com

@minrk
Copy link
Member

minrk commented Feb 15, 2014

Oh, I didn't see that you added a application/pdf->pdf transform in the javascript. That should probably not have been added. I'll make a PR to remove that.

@ellisonbg
Copy link
Member Author

I thought this was a originally a "not yet" thing. But once I got working,
I decided to make everything just work like other output formats. You can
view PDF output in the notebook UI and everything just works in nbconvert.
What will be the effect of that removal on these capabilities? What is the
plan to get the capabilities back?

On Sat, Feb 15, 2014 at 1:13 PM, Min RK notifications@github.com wrote:

Oh, I didn't see that you added a pdf->application/pdf transform in the
javascript. That should actually not have been added. I'll make a PR to
remove that.

Reply to this email directly or view it on GitHubhttps://github.com//pull/4920#issuecomment-35168009
.

Brian E. Granger
Cal Poly State University, San Luis Obispo
bgranger@calpoly.edu and ellisonbg@gmail.com

@minrk
Copy link
Member

minrk commented Feb 16, 2014

The "not yet" part is fixing the renames that we already do. That doesn't mean that any new formats should get their own rename, knowing that it will just be removed later. We can already have arbitrary mimetype output in the notebook document, so I don't think there's a reason to add a rename for pdf.

What will be the effect of that removal of these capabilities? What is the
plan to get the capabilities back?

No capabilities are affected. The only thing I removed in #5132 is renaming 'application/pdf' to 'pdf', which did not add anything.

@ellisonbg
Copy link
Member Author

OK, thanks for the clarification, sounds good.

On Sat, Feb 15, 2014 at 6:55 PM, Min RK notifications@github.com wrote:

The "not yet" part is fixing the renames that we already do. That doesn't
mean that any new formats should get their own rename, knowing that it
will just be removed later. We can already have arbitrary mimetype output
in the notebook document, so I don't think there's a reason to add a rename
for pdf.

What will be the effect of that removal of these capabilities? What is the
plan to get the capabilities back?

No capabilities are affected. The only thing I removed in #5132https://github.com/ipython/ipython/pull/5132is renaming 'application/pdf' to 'pdf', which did not add anything.

Reply to this email directly or view it on GitHubhttps://github.com//pull/4920#issuecomment-35175765
.

Brian E. Granger
Cal Poly State University, San Luis Obispo
bgranger@calpoly.edu and ellisonbg@gmail.com

mattvonrocketstein pushed a commit to mattvonrocketstein/ipython that referenced this pull request Nov 3, 2014
Adding PDFFormatter and kernel side handling of PDF display data
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.

None yet

6 participants