👌 IMPROVE: Math Parsing by chrisjsewell · Pull Request #217 · executablebooks/MyST-Parser

chrisjsewell · 2020-08-18T22:27:41Z

This centralises parsing configuration storage and validation, meaning it only has to be updated in a single place.
Also moved from using texmath_plugin to dollarmath_plugin

This centralises parsing configuration storage and validation, meaning it only has to be updated in a single place. Also moved from using `texmath_plugin` to `dollarmath_plugin`

codecov · 2020-08-18T22:30:18Z

Codecov Report

Merging #217 into master will decrease coverage by 0.20%.
The diff coverage is 85.24%.

@@            Coverage Diff             @@
##           master     #217      +/-   ##
==========================================
- Coverage   91.17%   90.97%   -0.21%     
==========================================
  Files          12       12              
  Lines        1281     1296      +15     
==========================================
+ Hits         1168     1179      +11     
- Misses        113      117       +4

Flag	Coverage Δ
#pytests	`90.97% <85.24%> (-0.21%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
myst_parser/cli/benchmark.py	`82.53% <ø> (ø)`
myst_parser/docutils_renderer.py	`93.16% <0.00%> (-0.86%)`	⬇️
myst_parser/sphinx_parser.py	`97.05% <80.00%> (-0.38%)`	⬇️
myst_parser/__init__.py	`90.32% <81.25%> (+5.02%)`	⬆️
myst_parser/main.py	`91.54% <89.74%> (-0.12%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9c4ddac...2054b9f. Read the comment docs.

chrisjsewell · 2020-08-19T01:42:59Z

Ok @executablebooks/ebpteam and @phaustin I have now hopefully fully solved the Math parsing/rendering.

Basically, I wrote a new markdown plugin in executablebooks/markdown-it-py@91bf225, which gives greater control over the parsing process, and fixes the internal escaping issue (as raised in jupyter-book/jupyter-book#750 (comment)).

Then here, I use that plugin and also re-configure mathjax such that it does not try to replicate this parsing, and so "respects" if the math paring is disabled and makes it consistent with e.g. TeX/PDF outputs.

See https://myst-parser--217.org.readthedocs.build/en/217/using/syntax.html#math-shortcuts, and in particular https://myst-parser--217.org.readthedocs.build/en/217/using/syntax.html#mathjax-and-math-parsing
Also all the option are specified in https://myst-parser--217.org.readthedocs.build/en/217/using/intro.html#myst-configuration-options

NOTE because myst_amsmath_enable=False by default, this will mean that anyone who currently has "bare" LaTeX in their HTML documentation, which is now being rendered by MathJax, will no longer have it rendered, until they set myst_amsmath_enable=True.
We can decide if we want this to be the case, or to set it to True by default in myst-nb and/or jupyter-book.

This will then feed into executablebooks/MyST-NB#226 and finally into jupyter-book.

This ensures that math is only parsed by the markdown parser (when enabled)

choldgraf · 2020-08-19T02:07:27Z

+
+Since these delimiters are how `sphinx.ext.mathjax` wraps the math content in the built HTML documents.
+
+To inhibit this override, set `override_mathjax=False`.


is there some way for people to provide their own extra mathjax overrides? several folks have asked about this in JB

Just use mathjax_config? Then now, they will also have to ensure myst_override_mathjax=False

choldgraf · 2020-08-19T02:08:17Z

the docs addition looks good to me, I don't have time right now to take a look at the implementation so perhaps @mmcky can give a review on that. Two quick thoughts:

Is there a reason not to make "don't parse math if there are dollars signs just before/after" the default?
We mention mathjax overrides and that mathjax is the default, but I'm not sure where there's an option to choose anything other than Mathjax. Am I missing something?

Other than those nits I think that this looks really nice!

chrisjsewell · 2020-08-19T02:12:20Z

Is there a reason not to make "don't parse math if there are dollars signs just before/after" the default?

Not sure what you mean by this? Are you talking about myst_dmath_allow_space or can you give an example?

chrisjsewell · 2020-08-19T02:14:57Z

We mention mathjax overrides and that mathjax is the default, but I'm not sure where there's an option to choose anything other than Mathjax. Am I missing something?

Yes sphinx.ext.imgmath, sphinx.ext.jsmath and https://github.com/hagenw/sphinxcontrib-katex

chrisjsewell · 2020-08-19T03:02:29Z

Right I'm going to merge this, so I can get on to other things, but feel free to have a look through @mmcky and we can always fix anything in post

mmcky · 2020-08-19T04:53:12Z


+There are a few other options available to control dollar math parsing:
+
+`myst_dmath_allow_space=False`, will cause inline math to only be parsed if there are no initial / final spaces, e.g. `$a$` but not `$ a$` or `$a $`.


@chrisjsewell this is on by default?

mmcky · 2020-08-19T04:53:50Z

+
+`myst_dmath_allow_space=False`, will cause inline math to only be parsed if there are no initial / final spaces, e.g. `$a$` but not `$ a$` or `$a $`.
+
+`myst_dmath_allow_digits=False`, will cause inline math to only be parsed if there are no initial / final digits, e.g. `$a$` but not `1$a$` or `$a$2`.


@chrisjsewell is this on by default?

See the default configuration here: https://myst-parser.readthedocs.io/en/latest/using/intro.html#myst-configuration-options and in the code:

MyST-Parser/myst_parser/main.py

Line 22 in 8206f2a

class MdParserConfig:

I'm open to discussion about these though 😄

as in at present the default is that $ a $ will be parsed as math

ah nice page. I just noticed you added (off by default) in a couple other places in the text and was wondering if you wanted that here as well?

Yeh good point

If having a default value of myst_dmath_allow_space=True provides a more robust (but less technically correct) parse of the text -- I think it is probably good to have on by default. What do you think?

provides a more robust (but less technically correct)

Well I don't know if there is a "technichally correct" syntax rule for this, since obviously its not covered by the CommonMark spec. Just to clarify, you think that $ a $ should or should not be recognised a math?

mmcky · 2020-08-19T04:55:45Z

+
+    md.options.update(
+        {
+            "enable_html_img": config.html_img_enable,


@chrisjsewell is there a reason to have different option names here?

Not really, I just did want to make a breaking change to the internal code. But yeh can be changed at some point

mmcky

@chrisjsewell love your work on this! Many people will really love the features added here.

In my testing -- the only other edge case I have found is:

Edge Cases

The following myst

What happens if I use $$
L_x = y
$$

produces

chrisjsewell · 2020-08-19T05:06:37Z

The following rst

Do you mean that this is what is produced using rst (with a sphinx extension), or that this example was taken from an rst document?

Currently the dollarmath plugin does not support inline double-dollar. It is on the TODO list though

mmcky · 2020-08-19T05:11:16Z

Do you mean that this is what is produced using rst (with a sphinx extension), or that this example was taken from an rst document?

Sorry old habits -- I mean myst

Currently the dollarmath plugin does not support inline double-dollar

OK Roger that. So the display math needs leading white space

These also produce the same output

What happens if I use $$L_x = y$$

What happens if I use
$$
L_x = y
$$

but this works:

$$
L_x = y
$$
What happens if I use

so leading white space is a hard requirement for displaymath -- makes sense

mmcky · 2020-08-19T05:14:35Z

We may want to update this doc page as the syntax used shows $$..$$ on the one line which may confuse people. Want me to add a note?

chrisjsewell · 2020-08-19T05:16:39Z

Yeh pop it in a PR cheers

Basically this is the current specification: https://raw.githubusercontent.com/executablebooks/markdown-it-py/master/tests/test_plugins/fixtures/dollar_math.md

chrisjsewell · 2020-08-19T05:17:58Z

If you (or anyone) feels there are any issues then altered/additional tests should be added to that document (which gets run a parametrized pytests)

mmcky · 2020-08-19T05:37:34Z

Basically this is the current specification: https://raw.githubusercontent.com/executablebooks/markdown-it-py/master/tests/test_plugins/fixtures/dollar_math.md

So I notice the one line syntax for display math works when next to a code-block but not text

numbered equation following code block. (valid=True)
.
```
code
```
$$a+b$$(1)
.
<pre><code>code
</code></pre>
<section>
<eqn>a+b</eqn>
<span class="eqno">(1)</span>
</section>

Do you want me to add a test case

numbered equation following text. (valid=False)
.
Immediately after text
$$a+b$$(1)

and document False for now?

chrisjsewell · 2020-08-19T06:02:25Z

yeh go for it

chrisjsewell · 2020-08-19T06:03:52Z

It wouldn't be too hard to add inline displaymath parsing. However, I would also need to test how sphinx would handle this

♻️ REFACTOR: Add MdParserConfig class

98573b9

This centralises parsing configuration storage and validation, meaning it only has to be updated in a single place. Also moved from using `texmath_plugin` to `dollarmath_plugin`

chrisjsewell changed the title ~~♻️ REFACTOR: Add MdParserConfig class~~ 👌 IMPROVE: Math Parsing Aug 19, 2020

chrisjsewell marked this pull request as ready for review August 19, 2020 01:21

chrisjsewell force-pushed the dollarmath branch from 98c18e3 to 2e651d4 Compare August 19, 2020 01:36

chrisjsewell requested a review from choldgraf August 19, 2020 01:43

chrisjsewell added 4 commits August 19, 2020 02:45

‼️ BREAKING: Add mathjax override

7416051

This ensures that math is only parsed by the markdown parser (when enabled)

📚 DOCS: Update math section

a91c9fe

🔧 MAINTAIN: Add codecov.yml

7d7802b

📚 DOCS: Update heading levels

6a58638

chrisjsewell force-pushed the dollarmath branch from 2e651d4 to 6a58638 Compare August 19, 2020 01:46

choldgraf reviewed Aug 19, 2020

View reviewed changes

🔧 MAINTAIN: Fix typo

2054b9f

chrisjsewell merged commit eab8d5b into master Aug 19, 2020

chrisjsewell deleted the dollarmath branch August 19, 2020 03:04

mmcky reviewed Aug 19, 2020

View reviewed changes


		Since these delimiters are how `sphinx.ext.mathjax` wraps the math content in the built HTML documents.

		To inhibit this override, set `override_mathjax=False`.


		There are a few other options available to control dollar math parsing:

		`myst_dmath_allow_space=False`, will cause inline math to only be parsed if there are no initial / final spaces, e.g. `$a$` but not `$ a$` or `$a $`.


		`myst_dmath_allow_space=False`, will cause inline math to only be parsed if there are no initial / final spaces, e.g. `$a$` but not `$ a$` or `$a $`.

		`myst_dmath_allow_digits=False`, will cause inline math to only be parsed if there are no initial / final digits, e.g. `$a$` but not `1$a$` or `$a$2`.

Conversation

chrisjsewell commented Aug 18, 2020

Uh oh!

codecov Bot commented Aug 18, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

chrisjsewell commented Aug 19, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

choldgraf commented Aug 19, 2020

Uh oh!

chrisjsewell commented Aug 19, 2020

Uh oh!

chrisjsewell commented Aug 19, 2020

Uh oh!

chrisjsewell commented Aug 19, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mmcky left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Edge Cases

Uh oh!

chrisjsewell commented Aug 19, 2020

Uh oh!

mmcky commented Aug 19, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mmcky commented Aug 19, 2020

Uh oh!

chrisjsewell commented Aug 19, 2020

Uh oh!

chrisjsewell commented Aug 19, 2020

Uh oh!

mmcky commented Aug 19, 2020

Uh oh!

chrisjsewell commented Aug 19, 2020

Uh oh!

chrisjsewell commented Aug 19, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codecov Bot commented Aug 18, 2020 •

edited

Loading

chrisjsewell commented Aug 19, 2020 •

edited

Loading

mmcky left a comment •

edited

Loading

mmcky commented Aug 19, 2020 •

edited

Loading