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

Support rendering tex even when global MathJax rendering mode is not SVG #2994

Merged
merged 10 commits into from
Oct 1, 2018

Conversation

jonmmease
Copy link
Contributor

This PR makes two small changes, that together make it possible for Plotly.js to successfully render Latex even when the global MathJax rendering mode is not SVG (For example, in the Jupyter Notebook)

Changes:

  1. Specify the SVG font explicitly
  2. Set rendering mode to SVG just prior to typesetting, then restore it to it's original value just afterwards

With this change, latex rendering works in the following MathJax combined configurations:

  • TeX-MML-AM_CHTML
  • TeX-MML-AM_HTMLorMML
  • TeX-MML-AM_SVG
  • TeX-AMS-MML_HTMLorMML
  • TeX-AMS_CHTML
  • TeX-AMS_SVG
  • TeX-AMS_HTML
  • TeX-AMS-MML_SVG

Previously it only worked in the TeX-AMS-MML_SVG configuration.

(Note: It still does not work if the global MathJax TeX extensions are not loaded, as in the case for the "AM_SVG" configuration for example.)

 1. Specify the SVG font
 2. Set rendering mode to SVG just prior to typesetting, then restore it to it's original value just afterwards

With this change, latex rendering works in the following MathJax configurations:
 - TeX-MML-AM_CHTML
 - TeX-MML-AM_HTMLorMML
 - TeX-MML-AM_SVG
 - TeX-AMS-MML_HTMLorMML
 - TeX-AMS_CHTML
 - TeX-AMS_SVG
 - TeX-AMS_HTML
 - TeX-AMS-MML_SVG

(It still does not work if the global MathJax TeX extensions are not loaded.)

Previously it only worked in the TeX-AMS-MML_SVG configuration. Also, with these changes latex
rendering works without any additional configuration when used from Python (and probably R) inside
the Jupyter Notebook.
if(originalRenderer !== 'SVG') {
MathJax.Hub.Queue(['setRenderer', MathJax.Hub, originalRenderer]);
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we make this decision synchronously?

function finalize() { ... /* all the stuff we do with the results */ }
var originalRenderer = MathJax.Hub.config.menuSettings.renderer;
if(originalRenderer !== 'SVG') {
    MathJax.Hub.Queue(['setRenderer', ...], ['Typeset', ...], ['setRenderer', ...], finalize);
}
else {
    MathJax.Hub.Queue(['Typeset', ...], finalize);
}

As is I'm concerned

  1. that it adds unnecessary overhead to the svg-only case
  2. there's a potential race condition if someone else adds to the queue before we reset the renderer.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The trouble is that MathJax.Hub.config.menuSettings.renderer isn't initialized right away, so if it's accessed outside of a Queue command it will be null in the beginning. Perhaps there's a better way to wait for the first initialization than putting the MathJax.Hub.config.menuSettings.renderer on the Queue, but I'm not sure at this point.

Let me know if you have any ideas, I'll try a few more experiments tonight...

Copy link
Collaborator

Choose a reason for hiding this comment

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

The trouble is that MathJax.Hub.config.menuSettings.renderer isn't initialized right away

Ah OK... I see it immediately but perhaps that's just due to how we loaded MathJax in the first place... but I guess even if it's present, there's a case where someone else has put something in the Queue that changes the renderer, so by the time our expression gets rendered we have a different value from the one we read synchronously.

OK, so perhaps we can't do anything about the overhead - MathJax has enough overhead anyway, this is probably the least of our problems - but the race condition is still there. Since we're already in a function being executed by the Queue, can we just call MathJax.Hub.setRenderer(originalRenderer) directly, right where you have it, rather than putting it in the Queue? Or I guess, to be even safer, move this to the end of our finalize function so we can return MathJax.Hub.setRenderer(originalRenderer) in the (presumably unlikely) event that this happens async.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I see what you mean about the race condition. I'll give that a try again, but for some reason when I was testing earlier the direct MathJax.Hub.setRenderer call (not through the Queue) wasn't working properly, but I don't remember in what context that was. I'll report back...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, it does work fine to end with return MathJax.Hub.setRenderer(originalRenderer) and remove the extra Queue command. Done in 1dd4fd3.

@alexcjohnson
Copy link
Collaborator

Thanks @jonmmease - I had gone looking for a way to query and reset the renderer a while back and failed... so thanks for finding this!

The mocks that include LaTeX have changed - presumably because of the explicit font. Presumably explicit is more robust than implicit, and this one looks to be slightly more compact which is probably better for backward compatibility than if it had gotten larger... @etpinard are you OK with this change? If so we just need to regenerate these baseline images.

related: #2300, #2403 - does this solve those issues, or is there more required? Can we write a test for this, something like:

MathJax.Hub.Queue(
    ['setRenderer', /* not SVG */ ... ],
    function() { 
        Plotly.newPlot(gd, /* something with MathJax */).then(function() {
            expect(renderer).toBe(/* whatever we set it to above */);
        })
       .catch(failTest).then(done);
    }
);

@jonmmease
Copy link
Contributor Author

To fix #2300 (and maybe #2403) I think Plotly.js would need this change, and nbconvert should also explicitly invoke typesetting, instead of relying on the default MathJax behavior of automatically typesetting everything.

On the other hand, maybe we don't need to disable automatic typesetting anymore. I'll try removing skipStartupTypeset: true and see if that breaks anything for Plotly.js with this change in place.

@etpinard etpinard added bug something broken status: in progress labels Sep 11, 2018
This should remove a potential race condition
@alexcjohnson
Copy link
Collaborator

On the other hand, maybe we don't need to disable automatic typesetting anymore. I'll try removing skipStartupTypeset: true and see if that breaks anything for Plotly.js with this change in place.

That might be a breaking change for some users. I would love it if we could figure out a way to allow you to invoke that mode (and/or other config changes that we would then reset just like you're doing here with the renderer), but that seems tricky given that we're making the current MathJax config call on load. I suppose we could imagine deferring that call to the first Plotly.plot, or even to the first time we encounter LaTeX, in which case there could be a plotly.js config parameter allowing you to modify the MathJax config...

@jonmmease
Copy link
Contributor Author

Are you thinking that removing the skipStartupTypeset: true might be a breaking change? The MathJax default is to perform automatic typesetting on startup, and it looks like nbconvert assumes this default behavior. So the skipStartupTypeset: true option is currently breaking nbconvert.

But when I remove the option, and let MathJax perform the auto typesetting on load, that seems to break the Plotly.js latex rendering, even with the setRenderer updates in this PR 😕

BTW, I've been testing this outside of the plotly.js project like this:

<!DOCTYPE html>
<html lang="en">
<head>
    <script type="text/javascript" src="https://cdnjs.cloudflare.com/ajax/libs/mathjax/2.7.5/MathJax.js?config=TeX-MML-AM_CHTML">
    </script>
    <script type="text/javascript" src="dist/plotly.min.js"></script>
    <meta charset="UTF-8">
    <title>Title</title>
</head>
<body>
    <div id="graph"></div>
    <div>
        <p>Some $$\LaTeX$$ math</p>
    </div>
</body>
<script>

    Plotly.plot('graph', [{y: [2,1,2]}],
        {title: '$f(x) = \\int_0^\\infty \\Psi(x, t) \\; \\text{d} t $'}).then(function() {
    });
    // MathJax.Hub.Queue(['Typeset', MathJax.Hub]);
</script>
</html>

Right now, with skipStartupTypeset: true the plotly figure renders latex properly but the Some $$\LaTeX$$ math expression isn't typeset unless I uncomment the MathJax.Hub.Queue(['Typeset', MathJax.Hub]); line to explicitly invoke global typesetting.

If I remove skipStartupTypeset: true, then Some $$\LaTeX$$ math is automatically typeset, but the latex rendering in Plotly.js doesn't show up (and there are no console errors).

This avoids race condition where an external typesetting operation
is in progress (or already on the Queue) at the start `texToSVG`
function.

With this change, we no longer need to set `skipStartupTypeset` to true,
which alters the global default MathJax behavior of typesetting the full page
automatically on startup. Other utilities, such as nbconvert, have
not worked properly with plotly because they rely on this default
behavior
@jonmmease
Copy link
Contributor Author

jonmmease commented Sep 12, 2018

Alright, I think I'm getting somewhere!

This could be a breaking change for Plotly.js users if they were relying on the presence of Plotly.js to disable MathJax's default behavior of running typesetting on startup. To disable this explicitly, a user would need to declare MathJax like this:

<script type="text/x-mathjax-config">
        MathJax.Hub.Config({skipStartupTypeset: true});
</script>
<script src="mathjax/MathJax.js?config=TeX-AMS-MML_SVG"></script>

and if we keep this change it would probably make sense to add this to the recommended configuration in dist/README.md.

  • Finally in 6cba8e3 I removed our one-time MathJax config in src/fonts/mathjax_config.js and I added a pair of commands to the MathJax Queue to save the original configuration, set our configuration, and then restore the original configuration. I can already see the benefit of this change in the Jupyter Notebook. The notebook (and nbconvert and sphinx) set
    displayAlign to 'centered' in their configuration, but we set it to 'left'. Previously, the presence of Plotly.js would cause all displayed latex in the notebook to be left aligned. But with this change they remain centered.

Fonts:
I think I also understand the situation surrounding the SVG font property. When MathJax is configured in an SVG mode this defaults to TeX, but it looks like Jupyter doesn't ship with this font (See jupyter/notebook#2536). It ships with STIX-Web, and according to the MathJax documentation "The STIX-Web font is the most complete".

If we want to lock in the font, I think it makes sense to go with STIX-Web. If we don't want to change the default behavior, then consumers of plotly.js in the Jupyter context would need to explicitly configure the SVG font to be STIX-Web themselves before calling Plotly.js.

Here's a font side-by-side

TeX:
tex

STIX-Web:
stix-web

Performance:
There is likely some level of performance degradation with these changes, but I don't know if its meaningful compared to MathJax's overall rendering time. I don't see any difference by eye when refreshing our mocks that use latex. Is there a good way to check this more rigorously?

Testing:
If we agree on this general approach of setting/restoring the MathJax state, I can start working on some tests to make sure that we actually do so.

@alexcjohnson
Copy link
Collaborator

@jonmmease that was some great detective work! If we were adding MathJax support today knowing what we know now, this would be the solution, no question. I might try some things to reduce the overhead, particularly for the case where plotly.js is the only MathJax consumer so the config setting/resetting is unnecessary... but you're probably right that this is a negligible portion of overall MathJax rendering time, so I wouldn't hold it up for that.

We need to think through the potential breaking change though - three bad things could happen:

  • MathJax incorrectly typesets content that either isn't math or was intended to be managed some other way.
    • This is mitigated by the fact that the initial typeset will use the default $$...$$ instead of the $...$ that we switch to. There's also \(...\) which doesn't change. These delimiters were chosen to be unlikely to appear naturally, with the intent to remain displayed, though one could always construct uses for them.
    • Would anyone ever insert partial math, to be completed and typeset later? Seems unlikely... would be a major FOUC, normally you'd just insert and typeset the whole expression when it's ready.
  • MathJax slows down startup just by looking for content. They claim this is negligible but I thought I'd mention it.
  • The user had adapted to our config, and was explicitly typesetting blocks using $...$ delimiters, and now these stopped working.

The first seems like an exceedingly rare problem; the second is purportedly negligible though I haven't measured it personally; the third is perhaps the most likely, though I get the impression that the number of users in this camp is far fewer than those who are currently unable to adapt to the config we've set, and these users (and any bothered by the first two issues) need only make a small change, reinstating the piece(s) of our config that they depended on as Jon shows above.

So I guess I would call this an acceptably-small break given the benefits and ease of adaptation for affected users. But would appreciate other perspectives, particularly from @etpinard.

@etpinard
Copy link
Contributor

So I guess I would call this an acceptably-small break given the benefits and ease of adaptation for affected users. But would appreciate other perspectives, particularly from @etpinard.

How hard would it be to make to cover @jonmmease 's brilliant new MathJax logic under a config flag? For example,

{
  MathJaxConfig: 'global' || 'local'
}

where 'global' remains the default in v1.x, and 'local' corresponds to the logic in this PR, the value for Jupyter, and the default in v2.

@alexcjohnson
Copy link
Collaborator

How hard would it be to make to cover @jonmmease 's brilliant new MathJax logic under a config flag

Problem is MathJax config happens now on loading plotly.js, not on plotting. So there are effects before any Plotly.* call.

@jonmmease
Copy link
Contributor Author

I don't really understand what's happening here, but MathJax seems to support configuration parameters as part of the src string (e.g. ?config=TeX-AMS-MML_SVG)

<script src="mathjax/MathJax.js?config=TeX-AMS-MML_SVG"></script>

Could we (and would we want to) do something similar for plotly.js?

<script src="path/to/plotly.js?MathJaxConfig=local"></script>

Although, this might not cover the JupyterWidgets case since that's packaged through webpack and you never write down the src string.

@jonmmease
Copy link
Contributor Author

jonmmease commented Sep 13, 2018

Some more observations/thoughts.

It appears that not all of the config settings can be set and unset successfully. For example, it doesn't look like it's possible for the outer context to use the TeX font and plotly.js to use STIX-Web. So if we don't set skipStartupTypeset to true, or if we are not the first component to use SVG rendering, then we may not get to choose our own font.

Also, if we don't disable startup typesetting then the annoying "MathJax rendering" notifications display in the bottom of the browser on load. These are currently disabled by setting messageStyle to 'none' in mathjax_config.

Proposal

Update: We decided against this, but for posterity...

So here's a proposal for a compromise between where we are, what we want, and what MathJax can do.

  1. Keep our current mathjax_config.js configuration file but remove the displayAlign and messageStyle properties.
    MathJax.Hub.Config({
        skipStartupTypeset: true,
        tex2jax: {
                inlineMath: [['$', '$'], ['\\(', '\\)']]
        },
    });
  1. Then for each rendering call we set the renderer to SVG and set the displayAlign and messageStyle properties as we want them.
        MathJax.Hub.Config({
            messageStyle: 'none',
            displayAlign: 'left',
        })

Then we restore the renderer and configuration afterward.

  1. We leave the SVG font unspecified

Rational

  1. skipStartupTypeset: true and these inline math delimiters are useful defaults that existing plotly.js users may be relying on and they are consistent with those set in the Jupyter notebook.

  2. On the other hand messageStyle: 'none' and displayAlign: 'left' are settings that are in conflict with those specified by jupyter, and it appears that MathJax allows us to set and unset them successfully.

  3. By not specifying the SVG font explicitly there is no change in default behavior and users still have the option of changing it themselves. And the plotly.js tests shouldn't need to be updated.

Implications

for plotly.js users: This would only be breaking if they were relying on the presence of Plotly.js to set the global alignment to left or to disable all MathJax messages. In exchange, plotly.js will play much better with other MathJax components, especially those with non-SVG rendering modes.

for plotly.py: Plotly.py will be responsible for making sure that MathJax is configured with an available SVG font before invoking plotly.js. This will be done in a small straightforward bit of extra logic for plotly.offline.plot, plotly.offline.iplot and FigureWidget.

for nbconvert / sphnix / etc.:. To work with and without plotly.js, I would recommend they too configure MathJax with skipStartupTypeset: true and then explicitly call MathJax.Hub.Queue(['Typeset', MathJax.Hub]); after everything is loaded. This isn't ideal, but currently they don't work at all with plotly.js so this will certainly be an improvement.

If all three of these projects/groups adapt in these ways, then I think plotly.js latex could finally be widely used in these important technical computing contexts.

Add a MathJaxConfig option that, when set to 'local', causes plotly.js
to bypass all global MathJax configuration. Regardless of the value of
MathJaxConfig, all MathJax rendering commands are surrounded by logic
to properly configure the MathJax properties that plotly.js needs.

When MathJaxConfig is not 'local', plotly.js will continue to setup
the global MathJax config as it always has. This is the default behavior
for backward compatibility.  When plotly.js is used in a situation
where it is not the only component using MathJax, then MathJaxConfig
should be set to 'local' to keep from overwriting the user's desired
global configuration.
dist/README.md Outdated
@@ -33,6 +33,19 @@ or the un-minified version as:

You can grab the relevant MathJax files in `./dist/extras/mathjax/`.

By default, plotly.js will modify the global MathJax configuration on load.
Copy link
Contributor

Choose a reason for hiding this comment

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

dist/README.md is a generated file, you'll need to move this block to

https://github.com/plotly/plotly.js/blob/master/tasks/stats.js

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in 57e32ba

@etpinard
Copy link
Contributor

OK to merge this @jonmmease @alexcjohnson ?

I think all our concerns were addressed.

@jonmmease
Copy link
Contributor Author

I'm happy with it 🙂

@alexcjohnson
Copy link
Collaborator

💃 🏆 🎉

@etpinard
Copy link
Contributor

etpinard commented Oct 1, 2018

Merging this thing!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug something broken feature something new
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants