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

Add a notebook cell style to latex conversion #992

Merged
merged 4 commits into from Apr 24, 2019

Conversation

@t-makaro
Copy link
Collaborator

t-makaro commented Apr 19, 2019

I believe that nbconvert used to have a style_notebook.tplx file that used a cell style similar to that of the actual notebook, but it was removed (I can't find when) because it used a lot of vspaces and hspaces, so it had difficulty with alignment. Over the past year and a half, I've written an entirely new implementation that is much, much simpler. Alignment is almost all done by latex, and there are no custom latex drawing commands (previously used to allow boxes to split across pages).

Additionally, this new cell style has text wrapping, so that code/output no longer falls off the page. Fixes #323. Code cells even have a nice visual indicator to show that the wrapping is not syntactic. A little bit of magic was required to get verbatim environments to text wrap, but it works.

Ideally, I think this should be the default, but maybe we should leave that until 6.0?

image

@t-makaro

This comment has been minimized.

Copy link
Collaborator Author

t-makaro commented Apr 19, 2019

This is a subset of my work from https://github.com/t-makaro/nb_pdf_template.

Also, in the early days of developing this, I found a bug that I couldn't name this file style_notebook.tplx. Doing so caused the conversion to fail for some obscure reason.

@t-makaro

This comment has been minimized.

Copy link
Collaborator Author

t-makaro commented Apr 19, 2019

CC @mgeier

@t-makaro t-makaro changed the title Re-add a notebook cell style to latex conversion Add a notebook cell style to latex conversion Apr 19, 2019
@mgeier

This comment has been minimized.

Copy link
Contributor

mgeier commented Apr 20, 2019

Thanks @t-makaro, this looks very nice!

If somebody wants to try this, I think that's how it is supposed to be used:

python3 -m nbconvert --to latex --template style_jupyter some-notebook.ipynb

IMHO this should be merged into the default template, it looks just so much better than the current situation!

I currently don't have access to a LaTeX installation, so I can't play with the details. According to your screenshot, the only thing I would change is the formatting of the prompt. This is how I've done it in nbsphinx: https://github.com/spatialaudio/nbsphinx/blob/9365707bd299f438b3642d6aedc4820d6b65bbc4/src/nbsphinx.py#L1658-L1663
You might be able to do a similar thing.

@jfbu helped me a lot with this, probably he has some ideas for this case, too?

@MSeal

This comment has been minimized.

Copy link
Collaborator

MSeal commented Apr 23, 2019

I am getting a lot of latex errors using this new template when passing the results to xelatex

They start with:

! LaTeX Error: \usepackage before \documentclass.

Attached is a dummy example I made to test it out (extension changed to .txt to make it attachable). The default template successfully converts to latex and then to pdf with a xelatex Untitled.tex command.

Untitled.txt

Is there something I am missing or an issue in the template code?

@t-makaro

This comment has been minimized.

Copy link
Collaborator Author

t-makaro commented Apr 23, 2019

Sorry, I've been away from my computer.

@MSeal This template is meant to be inherited from by setting the cell_style like in the article.tplx. I will push an update to make this the default, and I will try out @mgeier's suggestions on prompt alignment.

@t-makaro

This comment has been minimized.

Copy link
Collaborator Author

t-makaro commented Apr 23, 2019

I ending up having to pass slightly different prompt spacing depending on whether the prompt was for input, text output and other output (images/math etc.). I've now made this style the default, so this closes #323. Assuming tests pass, this is ready to merge.

@t-makaro

This comment has been minimized.

Copy link
Collaborator Author

t-makaro commented Apr 23, 2019

I had to adjust the regex in the test for the new template, and I added a new test to still test the style_ipython template.

@t-makaro t-makaro force-pushed the t-makaro:template branch from 5d91e28 to 28a643f Apr 23, 2019
@t-makaro

This comment has been minimized.

Copy link
Collaborator Author

t-makaro commented Apr 23, 2019

It seems that the reason that only some versions of python have failing tests is due to using an old version of texlive. Python 3.7 is using a version of texlive from 2015 whereas python 3.6 and below is using a version of texlive from 2013 that does have appear to have the tcolorbox package, but not up to date dependencies.

I'm not sure where to go from here.

@t-makaro

This comment has been minimized.

Copy link
Collaborator Author

t-makaro commented Apr 23, 2019

I think it is reasonable to assume that users have a more up-to-date version of the pgf package, since this package only requires one from 2008 or later, and pgf should be a fairly popular package to have a more recent version of since TikZ is built on top of it, and TikZ is a very popular package. It's also been 6 years since 2013.... In my experience having already deployed this template to lots of people, this has never been an issue.

@t-makaro

This comment has been minimized.

Copy link
Collaborator Author

t-makaro commented Apr 23, 2019

I think the python 3.7 environment is getting the slightly newer version of tex live since it is using: dist: xenial

@t-makaro t-makaro mentioned this pull request Apr 24, 2019
@mgeier

This comment has been minimized.

Copy link
Contributor

mgeier commented Apr 24, 2019

I've now tried it on one of my own notebooks and it looks indeed nicer than previously. But, if I may say so myself, the PDF output of nbsphinx still looks nicer (see for example https://readthedocs.org/projects/nbsphinx/downloads/pdf/0.4.2/#section.4).

A few observations:

  • The distance between a text paragraph and a following code cell is slightly too small.
  • Indenting the first line of text after a code cell looks strange.
  • I think it would look nicer if the prompts would use the same fixed-width font as is used in code cells.
  • If a cell output contains something large (e.g. an image) which causes a page break, the output prompt remains on the previous page (I remember this was very tricky to solve in nbsphinx, there's loads of discussion in spatialaudio/nbsphinx#218)

But all those are minor things, I think this is already a great improvement!

@MSeal

This comment has been minimized.

Copy link
Collaborator

MSeal commented Apr 24, 2019

@mgeier Do you think it's at a quality level for the release this week? I also tested it out and it seems like a strict improvement on what's there now. I think I'm a soft +1 to merge before the release and improve afterwardswith the incremental improvements over time after the bigger release.

@t-makaro

This comment has been minimized.

Copy link
Collaborator Author

t-makaro commented Apr 24, 2019

Addressing @mgeier's points:

  1. Not sure how I would fix this. Maybe tcolorbox has a setting for it. A vspace would also work, but I wish to avoid that, and it may be too much space when it's a title before the code cell.
  2. Markdown doesn't indent paragraphs whereas latex does. This can be fixed with a simple \usepackage{parskip}, but that should not be a part of the cell style. It should go in the top level article.tplx template, which is out of the scope of what I want to do with this PR. I do wish to later do a follow up PR, and port much of the rest of my work from nb_pdf_template. This cell style should work with any template.
  3. The prompts are technically inside the tcolorbox, but outside the verbatim. If I can figure out the font used for the verbatim, then I can apply it to the prompts. I can do this now, or I can leave it for a follow-up which I'm planning anyway.
  4. I believe this is technically an issue for the current template too. The prompt is just text before an image, so they get placed separately since they are not inside any sort of box.
@mgeier

This comment has been minimized.

Copy link
Contributor

mgeier commented Apr 24, 2019

@MSeal

Do you think it's at a quality level for the release this week? I also tested it out and it seems like a strict improvement on what's there now.

To be perfectly blunt, the PDF output has never really looked very good. And I agree, this PR is definitely an improvement. A large one, in fact. If it were up to me, I would include it as-is in the upcoming release.

@MSeal

This comment has been minimized.

Copy link
Collaborator

MSeal commented Apr 24, 2019

@mgeier I agree and thanks for providing the opinion. @t-makaro I'll double check with @mpacer before we finalize the last test fix PR for the release, but I think we'll merge this today and get it into the release.

@@ -1,10 +1,10 @@

% Default to the notebook output style
((=- Default to the notebook output style -=))

This comment has been minimized.

Copy link
@mpacer

mpacer Apr 24, 2019

Member

Great catch!

This comment has been minimized.

Copy link
@mpacer

mpacer Apr 24, 2019

Member

One thought is that might be too greedy though with the - since that consumes whitespace that would previously have been added by the set cell_style. I think in this case it makes sense, but we should be careful whenever adding or subtracting - to our delimiters.

This comment has been minimized.

Copy link
@t-makaro

t-makaro Apr 25, 2019

Author Collaborator

The latex comment has been annoying me for a long time. There are a few other places that contribute to an unnecessary amount of whitespace at the top of the generated latex file that also annoy me when working with the latex file (not that it really matters), but I figured that I would start with this one while I was in that file.

@mpacer mpacer self-requested a review Apr 24, 2019
@mpacer mpacer merged commit 9836f81 into jupyter:master Apr 24, 2019
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@t-makaro t-makaro deleted the t-makaro:template branch Apr 25, 2019
@MSeal MSeal added this to the 5.5 milestone Apr 25, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.