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

Pdflatex figurefixes #178

Merged
merged 3 commits into from
Apr 5, 2016
Merged

Pdflatex figurefixes #178

merged 3 commits into from
Apr 5, 2016

Conversation

fperez
Copy link
Member

@fperez fperez commented Dec 27, 2015

Mostly improvements to figure handling, but a few extra fixes as well, to improve the quality of PDF output out of the box.

With these fixes, I was able to generate the final Sloan report with the command jupyter nbconvert --to pdf <notebook-filename>, without any further manual modification.

Note to whoever does final merge: merge commit message should list credit to https://gist.github.com/rwst/1366514 by @rwst, which is the main logic of this PR, the rest are small tweaks.

…versions.

I suspect most users will find it to be a more readable and pleasant font than
CM in the majority of applications and use cases, and it ships by default with
every modern LaTeX installation.
Control that figures don't overflow the page by giving them a max width, and
removing the empty "Figure N:" captions that pandoc would leave beind.

Credit for this logic: https://gist.github.com/rwst/1366514
@rwst
Copy link

rwst commented Dec 28, 2015

Thanks for the note. I have no problem with this, please go ahead.

On Mon, 28 Dec 2015 00:39 Fernando Perez notifications@github.com wrote:

Mostly improvements to figure handling, but a few extra fixes as well, to
improve the quality of PDF output out of the box.

With these fixes, I was able to generate the final Sloan report with the
command jupyter nbconvert --to pdf , without any
further manual modification.

Note, commit b9a781a
b9a781a
uses code from https://gist.github.com/rwst/1366514 by @rwst
https://github.com/rwst. I've asked for his explicit authorization and
would rather hear from him before we merge. While he put the code out in
public and we're only grabbing a handful of lines (with attribution), I
would still rather wait to hear for him.

But we can still review the code/approach in the meantime.

You can view, comment on, or merge this pull request online at:

#178
Commit Summary

  • Change default font size to 11pt for article class.
  • Use Palatino instead of Computer Modern as default font for LaTeX
    conversions.
  • Figure improvements: max width to 80% of text and remove empty
    captions.
  • Fix ulem bug that was disabling italics in \emph{}, missing normalem
    option.

File Changes

Patch Links:


Reply to this email directly or view it on GitHub
#178.

@fperez fperez changed the title [Do not merge] Pdflatex figurefixes Pdflatex figurefixes Dec 28, 2015
@fperez
Copy link
Member Author

fperez commented Dec 28, 2015

Great, many thanks @rwst! I've updated the PR description accordingly. Credit in the corresponding commit linking to your original gist, please let us know if you'd like any changes made, happy to do so.

\usepackage{graphicx}
% We will generate all images so they have a width \maxwidth. This means
% that they will get their normal width if they fit onto the page, but
% are scaled down if they would overflow the margins.
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 already be handled line 204 and works correctly for me with pandoc 1.15.2 (with a default of min witdth 80% pagewidth, height 80% pageheight,to ensure extra-long image still fit.) So This would be a regression.

Copy link
Member Author

Choose a reason for hiding this comment

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

Mmh, it wasn't working for me... The reason I introduced this logic was precisely because when I started creating the Sloan report, I was getting all my figures to be absolutely huge, blowing way past the page borders. I'm on OS X, pandoc 1.15.2.1.

So, evidently, something wasn't quite right until I put this logic in, but we should sort it out first...

@Carreau
Copy link
Member

Carreau commented Dec 28, 2015

I guess what you want is to set no-caption (not to get the Figure X:) when there is actually no text in the markdown alt-text. That make sens. But as is this would introduce regression to nbconvert.

For the width/height things if it does not work for you we can try to debug it.

@fperez
Copy link
Member Author

fperez commented Dec 28, 2015

Yes, we need a solution for the case where there's no caption in the alt-text, to avoid the ugly empty Figure X: markers.

As far as the figure size, I'm happy to drop that commit, if it worked, but in my case, it didn't at all. Can you try to create the Sloan report without this PR at all, and see what you get? Just run nbconvert --to pdf with master? I got huge figures that oveflowed the page very badly.

@Carreau
Copy link
Member

Carreau commented Dec 29, 2015

As far as the figure size, I'm happy to drop that commit, if it worked, but in my case, it didn't at all. Can you try to create the Sloan report without this PR at all, and see what you get? Just run nbconvert --to pdf with master? I got huge figures that oveflowed the page very badly.

Can you send me the images you use ? I just have the ipynb. (see if I get a pixel for pixel PDF)

@fperez
Copy link
Member Author

fperez commented Dec 29, 2015

On Tue, Dec 29, 2015 at 10:36 AM, Matthias Bussonnier <
notifications@github.com> wrote:

Can you send me the images you use ? I just have the ipynb. (see if I get
a pixel for pixel PDF)

sent!

Fernando Perez (@fperez_org; http://fperez.org)
fperez.net-at-gmail: mailing lists only (I ignore this when swamped!)
fernando.perez-at-berkeley: contact me here for any direct mail

@Carreau
Copy link
Member

Carreau commented Dec 30, 2015

sent!

Ah indeed, my bad. The current handling of width/height apply only in outputs,not pandokized markdown.

I think we can just update that to max size={0.9\linewidth}{0.9\paperheight} for uniformity with outputs (or equivalent syntax).

@fperez dont' worry about that, we'll take care of it !

@fperez
Copy link
Member Author

fperez commented Dec 30, 2015

Ok, thanks! I played a lot with sizes, and I think a bit smaller than 90% would be a little better, perhaps 75% or 80%. Visually it makes them somewhat more inset from the text. 90% barely shows as an inset, think of a block quote, and it's typically more inset than 10% (keep in mind that a 10% inset has to be split into 5% on each side, so it's almost nothing visually on each side to differentiate it). So I'd argue for at least 80%...

@ellisonbg
Copy link
Contributor

+1 on 80%

On Wed, Dec 30, 2015 at 10:23 AM, Fernando Perez notifications@github.com
wrote:

Ok, thanks! I played a lot with sizes, and I think a bit smaller than 90%
would be a little better, perhaps 75% or 80%. Visually it makes them
somewhat more inset from the text. 90% barely shows as an inset, think of a
block quote, and it's typically more inset than 10% (keep in mind that a
10% inset has to be split into 5% on each side, so it's almost nothing
visually on each side to differentiate it). So I'd argue for at least 80%...


Reply to this email directly or view it on GitHub
#178 (comment).

Brian E. Granger
Associate Professor of Physics and Data Science
Cal Poly State University, San Luis Obispo
@ellisonbg on Twitter and GitHub
bgranger@calpoly.edu and ellisonbg@gmail.com

@minrk
Copy link
Member

minrk commented Dec 31, 2015

80% it is.

@takluyver takluyver added the format:LaTeX pertains to exporting to the LaTeX format label Jan 6, 2016
@minrk minrk added this to the 4.2 milestone Jan 22, 2016
@minrk
Copy link
Member

minrk commented Jan 25, 2016

Do we want to merge this and refine in master, or is there anything left to do in this PR?

@Carreau
Copy link
Member

Carreau commented Feb 22, 2016

:-( Need rebase now.

@minrk
Copy link
Member

minrk commented Apr 5, 2016

@fperez I think the conflict is only on the last commit, because the same fix had been found and merged in another PR. If you pop that, I think we can merge this.

@minrk minrk merged commit 2570d94 into jupyter:master Apr 5, 2016
@fperez
Copy link
Member Author

fperez commented Apr 5, 2016

thanks!

@fperez fperez deleted the pdflatex-figurefixes branch April 5, 2016 23:17
@Carreau
Copy link
Member

Carreau commented Apr 5, 2016

🍰

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
format:LaTeX pertains to exporting to the LaTeX format
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants