Skip to content

Conversation

mpacer
Copy link
Member

@mpacer mpacer commented Jul 21, 2017

This tiny change makes it possible to use labels in the live notebook by constantly clearing the id storage (which otherwise would stop it from rendering since the id would be present more than once).

@blink1073
Copy link
Contributor

JupyterLab counterpart: jupyterlab/jupyterlab#2715

@ian-r-rose
Copy link
Member

cf the long discussion in ipython/ipython#4113.

The issue with this approach is that the numbering is reset with every render of a cell. If you have multiple cells with equations, then you wind up with redundant numbering. When every cell has a single equation, then each equation winds up with an equation number of 1 (not a very useful numbering).
eqnumber
The conclusion reached in ipython/ipython#4113 was to decouple the two parts of this PR, and make equation numbering configurable (and probably off by default). Then you could make the equations have a global numbering. The downside of the global numbering (as you note), is if you try to retypeset an equation, it messes up the labeling and it won't render correctly at all. That is where the second part comes in: rather than try to clear the numbering before every typeset, just make it a user command accessible from the command palette. I probably agree with this approach.

@ian-r-rose
Copy link
Member

That said, equation numbers are very important and a long-requested feature. Thanks to @mpacer for pushing forwards on this!

@mpacer
Copy link
Member Author

mpacer commented Jul 21, 2017

So, to be clear, this was explicitly not enabling equation numbering. But rather it was eliminating the labels that were otherwise applied in a permanent form inside the notebook (meaning you couldn't render a cell with an equation with a label more than once without it breaking).

So because of that… it would be impossible to decouple this feature because it isn't about equation numbering.

It's worth noting that part of my journey to get to this point did get equation numbering working… but I turned it off for this PR. This is more generally useful, and doesn't run into the rerendering $C(n,2)-1$ issue that we have to deal with when rendering cells initially with the appropriate Preprocess and Rerender calls.

Also, getting numbering to work properly requires rerendering all mathjax and therefore markdown cells on every instance of cell movement (insertion, deletion, shifting, pasting) & every new render of a markdown cell.

If we were to do that, we'd have to add a rerender call to the deletion event. The other manipulation commands are taken care.

This has a double issue of our current way of handling mathjax having a 2 or 3 step rendering process which results in an unspleasant shifting in the page's vertical content as the different pieces of mathjax are renrendered multiple times.

Still this can be merged without dealing with any of those things.

@ian-r-rose
Copy link
Member

Ah, right, my mistake! I got the two PRs mixed up.
This PR would still break the behavior of this extension, which does have the behavior described above.

@mpacer
Copy link
Member Author

mpacer commented Jul 21, 2017 via email

@jfbercher
Copy link

@mpacer wrote

Also, getting numbering to work properly requires rerendering all mathjax and therefore markdown cells on every instance of cell movement (insertion, deletion, shifting, pasting) & every new render of a markdown cell.

This is almost the approach used in jupyter_latex_envs where proper numbering is achieved by resetting equation numbers of all markdown cells and instructing mathjax to reprocess everything, cf this code (cell movement is still not monitored).
Would be nice to have this in the core notebook.

@ian-r-rose
Copy link
Member

I agree @mpacer, this is pretty much a win for usability.

@mpacer
Copy link
Member Author

mpacer commented Jul 24, 2017

@jfbercher if you run your extension on this PR, does it break?

This is not intended to address equation numbering — only the labels; it happens that the cleanest way I found to do that was to use the reset equation numbers. But it'd be nice if it didn't break your extension.

@ian-r-rose did you test the equation-numbering extension and saw that it broke or were you only guessing that it would mess that up?

@gnestor
Copy link
Contributor

gnestor commented Jul 27, 2017

@mpacer Is this ready to merge?

@mpacer
Copy link
Member Author

mpacer commented Jul 27, 2017

It does the intended things. What I don't know is whether it breaks extensions or not or whether that is a deciding factor

@gnestor
Copy link
Contributor

gnestor commented Jul 27, 2017

It sounds like it will break functionality for this extension. (Please excuse my naïveté of this subject) It sounds like this feature is needed within the notebook context: users re-run cells so therefore typesetting along with anything else rendered as output should be able to be re-rendered. Any extensions or notebooks that somehow depend on equation label IDs will break as a result of this, but I think they pros outweigh the cons. @mpacer Unless you can think of a better solution that won't break existing notebooks/extensions, I think we should merge and get this in the 5.1 release.

@ian-r-rose
Copy link
Member

Not so naïve as to forget diacritics, @gnestor . I agree it should be merged for the 5.1 release. I am trying to test the equation extension, but am having some trouble with the notebook dev build. Have you seen errors about a missing preact and preact-compat?

@gnestor
Copy link
Contributor

gnestor commented Jul 27, 2017

@ian-r-rose Are they fatal errors or just missing sourcemap warnings (e.g. 404 GET /static/components/preact/preact.min.js.map (::1) 2.50ms referer=None)?

@ian-r-rose
Copy link
Member

They are fatal errors:
404 GET /static/components/preact/index.js (127.0.0.1) 11.98ms referer=http://localhost:8890/tree?token=1e9cb9f093db739142aefd9cbb51c8b7e4527d11c8a7aa46

@blink1073
Copy link
Contributor

@ian-r-rose, I've seen that and a git clean -dfx clears it up.

@ian-r-rose
Copy link
Member

Thanks! Clearing out notebook/static/components and reinstalling did the trick.

I can confirm that this does not break the equation renumbering extension, so I think this is an unqualified improvement.

@gnestor gnestor added this to the 5.1 milestone Jul 27, 2017
@gnestor gnestor merged commit dbda330 into jupyter:master Jul 27, 2017
@mpacer
Copy link
Member Author

mpacer commented Jul 27, 2017

w00t! I had hoped because their trigger with the renumbering was not embedded in utils but actually built on top of everything else that it should be fine.

Should we try to get equation numbering and that toolbar element available as an option in the core notebook? I'm not sure how traitlets get propagated up to the js side of things, but if someone can point me to a case where that is done, I'm happy to make a PR. It'd be a nice feature to have included in 5.1.

@gnestor
Copy link
Contributor

gnestor commented Jul 27, 2017

I'm not sure how traitlets get propagated up to the js side of things, but if someone can point me to a case where that is done, I'm happy to make a PR.

In ipywidgets, there is a manager base class that handles comm messages and a widget base class that uses the manager to provide a traitlets-like interface. The widgets subclass the widget base class and define a model and view (like in traitlets) and the view can access the model and render it. Here is an example: HTMLMathModel and HTMLMathView

@jfbercher
Copy link

jfbercher commented Jul 27, 2017

@mpacer I confirm, lately, that the PR does not beak jupyter_latex_envs nor equation-numbering nbextensions. It probably even improves equation-numbering which did not supported rerendering equations with labels.
Pinging @juhasch for equation-numbering.

@jfbercher
Copy link

@mpacer Regarding

our current way of handling mathjax having a 2 or 3 step rendering process which results in an unspleasant shifting in the page's vertical content as the different pieces of mathjax are renrendered multiple times.

Do you know where this is done and if there is a possibility to improve that?

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 6, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants