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

Use only already included latex envs in latex reprs #74

Closed
wants to merge 1 commit into from

Conversation

jankatins
Copy link
Contributor

@jankatins jankatins commented Jul 13, 2016

Unfortunately, we can't really influence what the notebook or better nbconvert
uses to include in the tex file and so set the defaults to something which works
out of the box.

Closes: IRkernel/IRkernel#331

Unfortunately, we can't really influence what the notebook or better nbconvert
uses to include in the tex file and so set the defaults to something which works
out of the box.
@jankatins
Copy link
Contributor Author

jankatins commented Jul 13, 2016

This is basically "Change until 'File > Download as > PDF' works". I don't think we can expect our users to run a IRdisplay::display_latex("....") before converting the pdf. And I really do not want to answer that over and over again :-(

@jankatins
Copy link
Contributor Author

A different option would be to use highr always and if that is not available (and asked for) send out a warning. I.e. remove the latex way completely.

@flying-sheep
Copy link
Member

See several comments everywhere this came up.

The LaTeX packages used in the default template are ad-hoc to a high degree. Fixing this by allowing to configure per-kerneltype templates or registering per-document globals (includepackage or HTML script tags) is more important than getting the download button to work.

We should include a FAQ entry though

@flying-sheep
Copy link
Member

Sorry, I know you'd rather see us being practical here, but this is a Jupyter/nbconvert shortcoming, and we can fix those in a short amount of time.

We just need to work together to draft and implement a real solution ASAP, OK?

@jankatins
Copy link
Contributor Author

jankatins commented Jul 13, 2016

I disagree: I don't think we have any chance to get this done with a FAQ until this is implemented upstream. Getting "File > download as > PDF" working out of the box (if that works with a default python notebook) is IMO a feature we cannot throw away until it is "fixed upstream". And getting it "fixed upstream" will be a huge change in the notebook server/ messaging spec/ ipynb file format that it will take at least a big version change (Seeing how long the kernel nany proposal takes, I suspect at least a year).

If the "default" latex mode is not working, then IMO we should disable latex generation completely (=plain text only for PDFs). If it needs a call to set something up, then this call should then also enable latex Generation. The current way (enable it and it looks like it is working, but it will break when a user uses the latex way) is IMO a bad service to our users.

This PR basically does not change a thing (it looks a bit different and I now have to install highr and enable it in the notebook instead of calling display function to load the latex package), BUT without any "enabling", I get a working PDF export. I've also no problem to add another option to enable enumerate*, but not as a default.

IMO the design philosophy should be "get it working in the default environment (plain jupyter notebook + nbconvert) now and everything else has to be enabled (manually or automatically)".

So, please, let us discuss this properly here before deciding on a solution.

@jankatins jankatins reopened this Jul 13, 2016
@flying-sheep
Copy link
Member

flying-sheep commented Jul 13, 2016

I can see where you're coming from but I disagree.

No huge changes are needed. A kernel specific template is pretty easy to implement. it isn't a perfect solution but better than “everyone uses the template someone hacked together for IPython’s specific needs”

My priorities are: 1. Get something that's suited and stable for the most common use cases 2. Help improve the ecosystem. for this, explore the possibilities and go new paths

My anti-priority is to compromise something that otherwise works well/fast/elegantly in order to workaround some other software’s shortcomings that break some minor feature.

This is why I was reluctant about the SVG change: SVG looks better and is losslessly zoomable, but because it's not the default, all other involved projects will continue to have subtly broken SVG handling, just as we witnessed so often while it was default. When I only activate SVG for myself, I'll file dozens of bugs that will resemble graveyards because nobody will care about a non-default output format.

“doctor if I touch here it hurts” – “then stop touching there”

But I want this shit fixed, what kind of lost doctor are you!

…But in the end, SVG is different since the browsers’ performance problems are too deep-seated to expect a comprehensive fix soon, so here I saw why we had to go the compromising-but-practical way.


I'm OK with adding an option for the vector environment as long as it's “enumerate*” by default. You can put it into your .Rprofile, and we'll add a FAQ entry

I'm not OK with using verbatim at all, since it has deep-started encoding issues that will only bring us and users pain.

@flying-sheep
Copy link
Member

flying-sheep commented Jul 13, 2016

OK, a bit more structured:

  • reprs’s scope is to create mime reprs

among them LaTeX, and LaTeX code usually needs specific packages to work

therefore we should be able to choose every code we want for reprs depending on what looks best

  • nbconvert’s scope is to convert a notebook to a document for mime type

for this it uses templates, yet there’s no way to specify per-document data for a mimetype, therefore no way to select packages

this means the packages it uses are unspecified and arbitrary

improper hackish solutions include:

  1. restrict repr to use only the arbitrary package subset that is currently in the default subset
  2. require users to specify a template

⇒ proper solutions include:

  1. include ALL THE PACKAGES in the default template (or continuously add stuff whenever people want)
  2. add a way to register per-document data that the template can use
  3. use kernel specific templates

@jankatins
Copy link
Contributor Author

jankatins commented Jul 13, 2016

for this, explore the possibilities and go new paths

I don't mind doing this, but in my opinion the goal for IRkernel and friends is to be "enterprise/production ready" as possible. That means my priorities are (in decreasing order!):

  1. get it running out of the box without errors and with as less manual config as possible
  2. get it nice and shiny
  3. help the ecosystem

Users don't read FAQs, they just switch to RStudio if it is not working and I don't want that. It was hard enough getting a nbconvert stack running on windows (now its "just" conda install notebook nbconvert miktex pandoc) but forcing the user to read docstrings or FAQs until it works is not acceptable.

@jankatins
Copy link
Contributor Author

Lets use this PR to discuss the goal perspective and the metadata stuff is in IRkernel/IRdisplay#14 and friends

@flying-sheep
Copy link
Member

flying-sheep commented Jul 13, 2016

this is the correct place for a fix. you see how they also added pandoc stuff right above where i inserted the package load statements?

if you still want to add the option, i’d be happy to review the change. you can either repurpose this PR or open a new one and we close this. as you want.

@jankatins
Copy link
Contributor Author

I still don't get it: now the whole jupyter world needs to install two more packages (miktex would be easy thanks to autoinstall, but for the rest the user has to touch tlmgr as it is not in e.g. texlive-latex-recommend on debian/ubuntu). What are the benefits of these two latex packages?

  • it's easier to install the highr package than to install a latex package -> you are already in the notebook, you know installing packages, you usually don't know how to install latex packages (one could even considere it a bug that setting highlight to false will highlight in pdfs).
  • a list is still a list?

Re lists: I made a comparison:

Notebook itself:
2016-07-14_010825

Without stars, plain notebook/nbconvert
2016-07-14_010906

With stars:
2016-07-14_011059

The "with-stars" is a bit more compact, but also much more difficult to read (-> list/description* latex env). (IMO in both cases the text plain variant (print(lst)) looks better as at least the output starts at the beginning of the line...)

@flying-sheep
Copy link
Member

flying-sheep commented Jul 14, 2016

i don’t care about ubuntu packaging decisions. it’s in texlive. not installing all of texlive will have you miss packages at random times. that’s why i never bothered to install just a subset after like one month of using LaTeX.

so my decision as package author is final: i will leave enumerate* as default for vectors.

@flying-sheep
Copy link
Member

flying-sheep commented Jul 14, 2016

about your comparison: enumitem is pretty awesome and flexible. you can change the looks at any point using \setlist

are you going to rework this to include the option or not? if not, i’ll do a new one introducing the option, and you can set it in your .Rprofile until jupyter/nbconvert#335 is merged.

@jankatins
Copy link
Contributor Author

I think we should wait until both the nbcovert maints and here @takluyver have said something. This also gives us time to calm down a bit...

@flying-sheep
Copy link
Member

sorry if i was too annoyed 😞

i really like you and your contributions here!

@jankatins
Copy link
Contributor Author

jankatins commented Jul 21, 2016

Can you give some pro argument of enumerate* other than "it looks prettier and is more flexible"?

In most cases, the latex files will only be used for PDFs, so "flexible" is only needed when you want to use the latex sources itself to be included in some other project (which I'm not sure if that's actually possible. Not sure if there is a way to write partial latex files.). Ok, you can also use a display_latex("...") in the notebook itself...

"prettier" is in the eye of the beholder, so I'm not going to argue this argument :-).

It currently breaks converting, not because it is not installed but because the usepackage is not included in the latex source. It will break until nbconvert implements any of the workarounds/fixes and gets a release: IRkernel is currently dev, so it is considered a moving target (although it is already shipped by conda), so I can agree that we break something, but I don't think we can expect people, especially R people to install nbconvert from source. Which means we are essentially shipping a "known to fail" software. :-(

IMO we should design this for "most" people, aka people who know R but not latex and just want a PDF. If you know enough latex to change the appearance of the lists then IMO it's also as easy to overwrite repr_latex.... to use a different template.

@flying-sheep
Copy link
Member

it’s a question of principle. i decided to output this LaTeX code which needs a certain package loaded, just like someone at some point decided to use \usepackage{enumerate} for some feature.

therefore the place to fix this is not here. there are multiple alternative fixes, with an increasing amount of generality, and note: they could all coexist, so no fix precludes another.

i know that the button doesn’t work. hasn’t for the longest time. you only discovered it recently because you happened to try it out. i promise you that most people haven’t and won’t use it and/or can wait a bit for a fix to ship in the correct place. until then there’s the template which we can put in the FAQ.

@jankatins
Copy link
Contributor Author

jankatins commented Jul 21, 2016

it’s a question of principle. i decided to output this LaTeX code which needs a certain package loaded, just like someone at some point decided to use \usepackage{enumerate} for some feature.

Sorry, but this is not an argument :-/ "Just because" is not a reason to leave a bug in if the "fix" on our side has no practical difference for people.

IMO new features have to work in the environment they are used and this means our side has to use what's available in nbconvert and if you want to change that you have to first get nbconvert to change and then use the new features here when they are available.

[I do agree that the last "best fix" is actually the way to go, but this is IMO at least one nbconvert version away because it involves a messaging spec change]

(this should be done if we want a fix ASAP)

ASAP is maybe half a year away if this isn't backported (and only if it is adopted)!

i know that the button doesn’t work. hasn’t for the longest time. you only discovered it recently because you happened to try it out.

Most people don't use it probably because latex and pandoc are required to make it work. Getting this to work is quite a problem, at least on windows. But now conda has both of them on conda-forge, so I hope that this is going to improve.

I suspect that the easiest workaround is actually removing any latex vector output by simply using print(vec) in the cell.

@flying-sheep
Copy link
Member

flying-sheep commented Jul 21, 2016

maybe we can lobby to get this into the patch branch, else it’s time to wait for half a year.

this project is not the place to fix it. i’m very sorry that it has to be this way, but for repr’s output quality and for the betterment of the jupyter environment, enumerate* will stay.

@jankatins jankatins closed this Aug 14, 2016
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.

2 participants