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

ENH: added StackExchange-style MathJax filtering #2349

Merged
merged 4 commits into from
Oct 20, 2012

Conversation

ahmadia
Copy link
Contributor

@ahmadia ahmadia commented Aug 27, 2012

Attempts to fix Issue #2289
Resubmission of accidentally merged Pull Request #2337

@travisbot
Copy link

This pull request passes (merged 0af930d7 into 011222a).

@fperez
Copy link
Member

fperez commented Sep 6, 2012

This is great! First, I'm testing the problem blocks listed in #2289. The following in a cell:

## Problem 1

$$
\begin{array}{c}
y_1 \\\
y_2 \mathtt{t}_i \\\
z_{3,4}
\end{array}
$$

## Problem 2

$$
\begin{array}{c}
y_1 \cr
y_2 \mathtt{t}_i \cr
y_{3}
\end{array}
$$

## Problem 3

$$\begin{eqnarray} 
x' &=& &x \sin\phi &+& z \cos\phi \\
z' &=& - &x \cos\phi &+& z \sin\phi \\
\end{eqnarray}$$

## Problem 4

Inline math $\mathbf{A}_{i}B_{j}$.

produces this image:

img

while master gives:

img

So this is obviously major progress!

Thanks a lot @ahmadia, I'll have a look now at the code. Pinging @minrk in case he's OK considering this for a 0.13.1 backport, as I think this is super high value.

deTilde;
if (hasCodeSpans) {
text = text.replace(/~/g, "~T").replace(/(^|[^\\])(`+)([^\n]*?[^`\n])\2(?!`)/gm, function (wholematch) {
return wholematch.replace(/\$/g, "~D");
Copy link
Member

Choose a reason for hiding this comment

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

It seems to me these nested function declarations would be more readable if they were broken up across lines. Right now these lines (above and below this comment) are really hard to read.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I'm getting used to a new editor, and one thing that it doesn't do automatically is hard line breaks. Does the javascript follow an 80 character line break?

@fperez
Copy link
Member

fperez commented Sep 6, 2012

The code is very non-trivial, with a ton of regex logic, but it looks clean and well-comented. I wish dearly we had some unit test machinery in place for the JS code so this could also get tested, as it's a bit too monolithic for my taste (and hitting it with a test harness would likely lead to some refactoring into more digestible pieces).

But given the current state of our JS testing (aka none), the importance of this fix and the fact that at least running it over many notebooks I have seems to always do the right thing, I'm +1 on merging it (once the very minor comments above are addressed). I will however wait a couple of days in case others have a different opinion. @ellisonbg, @Carreau, @minrk: you guys have looked the most at the JS code; any dissenting views here or other concerns?

Thanks for the contribution!

@ellisonbg
Copy link
Member

I will try to have a look at it

Sent from my iPad

On Sep 5, 2012, at 8:17 PM, Fernando Perez notifications@github.com wrote:

The code is very non-trivial, with a ton of regex logic, but it looks clean and well-comented. I wish dearly we had some unit test machinery in place for the JS code so this could also get tested, as it's a bit too monolithic for my taste (and hitting it with a test harness would likely lead to some refactoring into more digestible pieces).

But given the current state of our JS testing (aka none), the importance of this fix and the fact that at least running it over many notebooks I have seems to always do the right thing, I'm +1 on merging it (once the very minor comments above are addressed). I will however wait a couple of days in case others have a different opinion. @ellisonbg, @Carreau, @minrk: you guys have looked the most at the JS code; any dissenting views here or other concerns?

Thanks for the contribution!


Reply to this email directly or view it on GitHub.

@minrk
Copy link
Member

minrk commented Sep 6, 2012

Given the untested nature of our js, I would be a bit squeamish putting this in 0.13.1, with only a couple of days of testing.

@ahmadia
Copy link
Contributor Author

ahmadia commented Sep 6, 2012

I'm a little less squeamish about this code for the reason that I'm
intercepting PageDown in basically the same way that the StackOverflow guys
do it (my contribution/changes are the incorporation of the cross-browser
regex split function), but I agree that it's pretty easy to break major
functionality with bad javascript.

Fernando, if you're really interested in seeing this is 0.13.1, we could
see about binding it to either a keystroke or a dropdown menu item,
allowing the user to enable/disable MathJax-aware Markdown.

The biggest most-dangerous change I'm introducing is overriding the
javascript String.prototype.split method with the cross-browser code. This
function belongs somewhere obvious and bold (not at the top of
textcell.js), because it fundamentally changes the way the regex split
method will behave across the browser tab. In fact, thinking more
defensively about this, we shouldn't modify the base String object's split
method at all, and simply use the replacement function through an external
API.

I'll wait for you to guys to come to a decision about what you want and
where.

A

On Thu, Sep 6, 2012 at 6:08 AM, Min RK notifications@github.com wrote:

Given the untested nature of our js, I would be a bit squeamish putting
this in 0.13.1, with only a couple of days of testing.


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

@ellisonbg
Copy link
Member

Here is my proposal about where the code should go:

  • Put the split code into a function in utils.js.
  • Rename the current file initmathjax.js to mathjaxutils.js.
  • Put the other functions into mathjaxutils.js and call them at the appropriate place in textcell.js.

@ahmadia
Copy link
Contributor Author

ahmadia commented Sep 9, 2012

*On Fri, Sep 7, 2012 at 9:03 PM, Brian E. Granger notifications@github.com
wrote:
*

Here is my proposal about where the code should go:

  • Put the split code into a function in utils.js.
  • Rename the current file initmathjax.js to mathjaxutils.js.
  • Put the other functions into mathjaxutils.js and call them at the
    appropriate place in textcell.js.

I agree with Brian's proposal. I will perform the suggested modifications
and force-push another rebased branch (hopefully without merging it first
this time :) I am going to disable the change-split-everywhere convenience
modification and instead leave it exposed as a separate function.

I am waiting on the dev team to decide whether they want this backported to
0.13 and/if they want to add some "protection" via buttons or other
configuration over the remainder of the functionality (pre/postprocessing
math from the code before sending to the Markdown HTML formatter).

Regards,
Aron

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

@fperez
Copy link
Member

fperez commented Sep 9, 2012

On Sun, Sep 9, 2012 at 5:51 AM, ahmadia notifications@github.com wrote:

I agree with Brian's proposal. I will perform the suggested modifications
and force-push another rebased branch (hopefully without merging it first
this time :) I am going to disable the change-split-everywhere convenience
modification and instead leave it exposed as a separate function.

Great, thanks!!

I am waiting on the dev team to decide whether they want this backported
to
0.13 and/if they want to add some "protection" via buttons or other
configuration over the remainder of the functionality (pre/postprocessing
math from the code before sending to the Markdown HTML formatter).

Min was -1 on it, and since he's done all the work on making 0.13.1
possible by writing the backport tools and actually tracking
backport-worthy PRs and patches, I'm happy to defer to him. I'm personally
somewhat comfortable with it simply because its only role is to render
things correctly and I did test a lot of notebooks with it without issue;
but it is very true that so many things that look safe do blow up in
unexpected manners, that caution may be the wiser path to take here.

If we had a solid unittest infrastructure for the JS code I think this
discussion would go a different way, but such is the current state of
things, unfortunately.

Cheers,

f

@minrk
Copy link
Member

minrk commented Sep 9, 2012

Min was -1 on it

I would put it closer to -0.75 :)

There are really two considerations:

  1. js is untested in general, and since we are only days away from cutting 0.13.1, and this still isn't in master yet, the total 'in the wild' testing is really zero.
  2. To get into certain package environments, 0.13.1 must remain a strict bugfix branch. While this does fix an annoying issue, it is a much more substantial change to logic than any other in 0.13.1. I will defer to @juliantaylor on whether this change would be a problem for strict package policies.

@juliantaylor
Copy link
Contributor

I filed a preapproval of the 0.13.1 diff with the debian release team a while back but no answer yet (bug 686604), so I can't say much about the current chances of the branch with or without this change.
Ftw, I would prefer the change to be in master for a while before it is added to the bugfix branch.

@ellisonbg
Copy link
Member

ahmadia: what is the status of this, do you have more work to do to address the comments or are you waiting for further review?

@ahmadia
Copy link
Contributor Author

ahmadia commented Oct 2, 2012

@ellisonbg - the status is that @ahmadia needs to reimplement this properly following your suggestions, and has not found the time to do so, but at least has been feeling guilty about it. I'll try to have the updated in by Thursday, thanks for the nudge :)

@ellisonbg
Copy link
Member

@ahmadia please don't feel guilty - I was just making sure you weren't
waiting on me to do code review - that would make me feel guilty myself :)
Thanks for the code!

On Tue, Oct 2, 2012 at 1:33 PM, ahmadia notifications@github.com wrote:

@ellisonbg https://github.com/ellisonbg - the status is that @ahmadiahttps://github.com/ahmadianeeds to reimplement this properly following your suggestions, and has not
found the time to do so, but at least has been feeling guilty about it.
I'll try to have the updated in by Thursday, thanks for the nudge :)


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

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

@ahmadia
Copy link
Contributor Author

ahmadia commented Oct 9, 2012

Okay, I've got @ellisonbg 's suggested refactor in, but now I'm wrestling with getting MathJaX to properly process environments. All of the basic stuff is enabled here: https://github.com/ahmadia/ipython/tree/mathjax_refactor

What I'd like to do is get some of the cooler equation numbering and such going, but this is going to take some debugging (which is new to me): http://cdn.mathjax.org/mathjax/latest/test/sample-eqrefs.html

@ahmadia
Copy link
Contributor Author

ahmadia commented Oct 9, 2012

@ellisonbg @fperez @minrk - This pull request is once more ready for review. I've changed the default mathjax configuration so it accepts LaTeX environments, added AMS-style equation references and numbering, and significantly cleaned up the code module.

You no longer need $$ to trigger an environment, you can just use Markdown+MathJaX like the following:

In equation \eqref{eq:sample}, we find the value of an
interesting integral:

\begin{equation}
  \int_0^\infty \frac{x^3}{e^x-1}\,dx = \frac{\pi^4}{15}
  \label{eq:sample}
\end{equation}

In equation \eqref{eq:sample2}, we find the value of the *same*
interesting integral:

\begin{equation}
  \int_0^\infty \frac{x^3}{e^x-1}\,dx = \frac{\pi^4}{15}
  \label{eq:sample2}
\end{equation}

Using $$ has the nice advantage of not numbering equations. Of course you, can also use equation*.

@ellisonbg
Copy link
Member

Can you create a notebook in docs/examples/notebooks that shows how to use
all of this. Can't wait to review it.

Cheers,

Brian

On Tue, Oct 9, 2012 at 9:40 AM, ahmadia notifications@github.com wrote:

@ellisonbg https://github.com/ellisonbg @fperezhttps://github.com/fperez
@minrk https://github.com/minrk - This pull request is once more ready
for review. I've changed the default mathjax configuration so it accepts
LaTeX environments, added AMS-style equation references and numbering, and
significantly cleaned up the code module.

You no longer need $$ to trigger an environment, you can just use
Markdown+MathJaX like the following:

In equation \eqref{eq:sample}, we find the value of an
interesting integral:

\begin{equation}
\int_0^\infty \frac{x^3}{e^x-1},dx = \frac{\pi^4}{15}
\label{eq:sample}
\end{equation}

In equation \eqref{eq:sample2}, we find the value of the same
interesting integral:

\begin{equation}
\int_0^\infty \frac{x^3}{e^x-1},dx = \frac{\pi^4}{15}
\label{eq:sample2}
\end{equation}

Using $$ has the nice advantage of not numbering equations. Of course
you, can also use equation*.


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

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

@ahmadia
Copy link
Contributor Author

ahmadia commented Oct 10, 2012

@ellisonbg - I added a sample notebook. Note that nbviewer won't typeset it properly until we've correctly patched it.

// Some magic for deferring mathematical expressions to MathJaX
// Some of the code here is adapted with permission from Stack Exchange Inc.

var inline = "$"; // the inline math delimiter
Copy link
Member

Choose a reason for hiding this comment

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

Don't know how I feel about top level attributes. You have protected them using the JS module pattern though. The alternative would be to create a class that has all of this logic. @Carreau what do you think about this?

@ellisonbg
Copy link
Member

So this is looking pretty good now. A few comments:

  • The code is pretty complex and it is difficult to tell from just looking at it that it will work. How much of this is taken from StackExchange? I do like the sample notebook that demonstrates the capabilities though.
  • Please fix the naming of things: likeThis -> like_this.
  • What version of MathJax is required for the new features like equation numbering?

// Don't allow math to pass through a double linebreak
// (which will be a paragraph).
//
function removeMath(text) {
Copy link
Member

Choose a reason for hiding this comment

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

We are trying to use the var foo = function () style of function defs, please convert to this.

@ellisonbg
Copy link
Member

I just tried this branch and it is completely broken with the version of MathJax I am using. @Carreau @minrk @fperez how should we handle MathJax versioning issues? Sort of difficult.. Minimally, we should make sure that the install_mathjax function will only install versions that work with the current code base.

@ellisonbg
Copy link
Member

I just updated to v2.0 of MathJax and it is still broken. I see a JS console error:

Uncaught Error: Can't make callback from given data, MathJax.js:29

The notebook doesn't even finish loading.

@ellisonbg
Copy link
Member

Got it, you have to clear your browser cache when updating MathJax. The example notebook looks spectacular!

@ahmadia
Copy link
Contributor Author

ahmadia commented Oct 11, 2012

The code is pretty complex and it is difficult to tell from just looking at it that it will work. How much of this is taken from StackExchange? I do like the sample notebook that demonstrates the capabilities though.

The majority of the logic for stripping TeX/LaTeX before sending text to PageDown then replacing it after PageDown has rendered the Markdown-formatted text to HTML-formatted text comes from the StackExchange javascript source. Unfortunately, the javascript is no longer available from the URL I originally accessed it at, but was linked to from this question. I improved the source code by incorporating a more robust regular expression parser with explicit licensing. The StackExchange code is not copyright, but it is also not released in any way. I was given permission by the moderator team in a StackExchange logged and hosted chat session to reuse the code.

As pointed out on the discussion list, it would be great if we had a better Markdown parser than Pagedown, but until the competing projects are more mature, I think we are better off piggy-backing larger projects like StackExchange.

Are there any other concerns beside the naming issues?

thisStyle      --> this_style.
function foo() --> var foo = function()

StackExchange improperly attributed for Davide Cervone's
Markdown+MathJax handling.  This has been fixed.

Ref:
http://stackoverflow.com/a/11231030/122022
http://www.math.union.edu/~dpvc/transfer/mathjax/mathjax-editing.js
@ahmadia
Copy link
Contributor Author

ahmadia commented Oct 13, 2012

@ellisonbg - ready for review.

@ellisonbg
Copy link
Member

OK I think this looks good, merging.

ellisonbg added a commit that referenced this pull request Oct 20, 2012
ENH: added StackExchange-style MathJax filtering
@ellisonbg ellisonbg merged commit d66b481 into ipython:master Oct 20, 2012
@Carreau
Copy link
Member

Carreau commented Oct 21, 2012

Am I the only one to get a totally broken notebook after this merge ? With mathjax.js line 29 throwing Uncaught Error: Can't make callback from given data ?

@Carreau
Copy link
Member

Carreau commented Oct 21, 2012

FYI, it break when user have old mathjax version installed locally, they then need to upgrade to 2.0, check that the old version is removed (new version is installed in profile) and clear cache.

@ahmadia
Copy link
Contributor Author

ahmadia commented Oct 21, 2012

Brian pointed that out in the PR, and I didn't properly fix it :(

A

On Sun, Oct 21, 2012 at 4:20 PM, Bussonnier Matthias <
notifications@github.com> wrote:

FYI, it break when user have old mathjax version installed locally, they
then need to upgrade to 2.0, check that the old version is removed (new
version is installed in profile) and clear cache.


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

@ahmadia
Copy link
Contributor Author

ahmadia commented Oct 21, 2012

@Carreau @ellisonbg - I'm not familiar with the bit of code responsible for local MathJax installs. Is this something that some attention from me can help, or should I leave it to one of you to fix?

@ahmadia
Copy link
Contributor Author

ahmadia commented Oct 21, 2012

@Carreau - One thing I think I can do is try and detect if the MathJax library is out of date and issue an alert to the user. Would this be useful?

@ellisonbg
Copy link
Member

Yes, I would put this information in the mathjaxinit function. There is a
version attribute of MathJax namespace and its value should be "2.0". If
it doesn't match that, let's pop up a jquery dialog displaying a message
that the user needs to update MathJax. Note, they will probably also have
to clear their browser cache after doing the update.

On Sun, Oct 21, 2012 at 9:20 AM, ahmadia notifications@github.com wrote:

@Carreau https://github.com/Carreau - One thing I think I can do is try
and detect if the MathJax library is out of date and issue an alert to the
user. Would this be useful?


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

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

@Carreau
Copy link
Member

Carreau commented Oct 21, 2012

Yes, warning would be usefull I think, but number of user with local mathjax is IMHO small.
As for mathjax installing, you could have a look at #2446 and #2267 to see how it is done.

@ellisonbg
Copy link
Member

I am seeing two problems related to this branch:

  • Rendering in notebook load is really slow and is blocking the notebook UI.
  • I am still seeing the error (Uncaught Error: Can't make callback from given data, MathJax:29 ) when I reload a notebook with equations in it. Can others try this out.

@Carreau
Copy link
Member

Carreau commented Oct 24, 2012

I am still seeing the error (Uncaught Error: Can't make callback from given data, MathJax:29 ) when I reload a notebook with equations in it. Can others try this out.

For me only sometimes.

@ellisonbg
Copy link
Member

Yes, it is inconsistent for me too - probably a timing issue or something.

On Wed, Oct 24, 2012 at 10:05 AM, Bussonnier Matthias <
notifications@github.com> wrote:

I am still seeing the error (Uncaught Error: Can't make callback from
given data, MathJax:29 ) when I reload a notebook with equations in it. Can
others try this out.

For me only sometimes.


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

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

@ketch
Copy link

ketch commented Oct 25, 2012

After installing this, I also see very slow math rendering or (often, not always) the notebook doesn't seem to load at all; I just get a single cell that says "Type Markdown and LaTeX: α2". I see both effects in both Safari and Chrome.

I would love to have this functionality, though. It's very annoying that with 0.13 I can't cut/paste valid laTeX in ipython notebooks. But for now this has forced me to roll back to the release version.

@ellisonbg
Copy link
Member

I see the same things.

On Thu, Oct 25, 2012 at 5:23 AM, David Ketcheson
notifications@github.comwrote:

After installing this, I also see very slow math rendering or (often, not
always) the notebook doesn't seem to load at all; I just get a single cell
that says "Type Markdown and LaTeX: α2".

I would love to have this functionality, though. It's very annoying that
with 0.13 I can't cut/paste valid laTeX in ipython notebooks.


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

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

@ahmadia
Copy link
Contributor Author

ahmadia commented Oct 25, 2012

Thanks for feedback and apologies for the bug. I'm improving the logic
with regards to rendering, will report back soon.

A

On Thu, Oct 25, 2012 at 5:47 PM, Brian E. Granger
notifications@github.comwrote:

I see the same things.

On Thu, Oct 25, 2012 at 5:23 AM, David Ketcheson
notifications@github.comwrote:

After installing this, I also see very slow math rendering or (often,
not
always) the notebook doesn't seem to load at all; I just get a single
cell
that says "Type Markdown and LaTeX: á2".

I would love to have this functionality, though. It's very annoying that
with 0.13 I can't cut/paste valid laTeX in ipython notebooks.

Reply to this email directly or view it on GitHub<
https://github.com/ipython/ipython/pull/2349#issuecomment-9775424>.

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

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

@ellisonbg
Copy link
Member

Thanks!

On Thu, Oct 25, 2012 at 11:21 AM, ahmadia notifications@github.com wrote:

Thanks for feedback and apologies for the bug. I'm improving the logic
with regards to rendering, will report back soon.

A

On Thu, Oct 25, 2012 at 5:47 PM, Brian E. Granger
notifications@github.comwrote:

I see the same things.

On Thu, Oct 25, 2012 at 5:23 AM, David Ketcheson
notifications@github.comwrote:

After installing this, I also see very slow math rendering or (often,
not
always) the notebook doesn't seem to load at all; I just get a single
cell
that says "Type Markdown and LaTeX: á2".

I would love to have this functionality, though. It's very annoying
that
with 0.13 I can't cut/paste valid laTeX in ipython notebooks.

Reply to this email directly or view it on GitHub<
https://github.com/ipython/ipython/pull/2349#issuecomment-9775424>.

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

Reply to this email directly or view it on GitHub<
https://github.com/ipython/ipython/pull/2349#issuecomment-9784917>.


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

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

@ahmadia
Copy link
Contributor Author

ahmadia commented Oct 25, 2012

See #2517 for several superficial fixes and the fix for the notebook. Note that I've also turned rendering off for placeholder cells, we can reenable this pretty trivially (comment in #2517 on the fix suggested there).

fperez added a commit that referenced this pull request Nov 2, 2012
Addresses the javascript errors resulting in a blank notebook pointed out in #2349 as well as significant performance degradations that were introduced by that merge.

It also disables equation numbering/references, fixes broken offline access and re-typesets individual Notebook Cells instead of the entire document on edit.

Closes #2289.
@ahmadia ahmadia deleted the mathjax_fix branch May 17, 2013 10:18
mattvonrocketstein pushed a commit to mattvonrocketstein/ipython that referenced this pull request Nov 3, 2014
ENH: added StackExchange-style MathJax filtering
mattvonrocketstein pushed a commit to mattvonrocketstein/ipython that referenced this pull request Nov 3, 2014
Addresses the javascript errors resulting in a blank notebook pointed out in ipython#2349 as well as significant performance degradations that were introduced by that merge.

It also disables equation numbering/references, fixes broken offline access and re-typesets individual Notebook Cells instead of the entire document on edit.

Closes ipython#2289.
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.

8 participants