[latex] remove svg from latex priority

[mmcky]

This PR:

• removes image/svg+xml as a latex image option.

[mmcky]

@choldgraf I am not entirely sure how the solution xml is generated here. Do you run locally and visually inspect the solutions to update it?

E       AssertionError: FILES DIFFER:
39
E       /tmp/pytest-of-runner/pytest-0/test_complex_outputs_latex0/test_transform/test_complex_outputs_latex.xml
40
E       /tmp/pytest-of-runner/pytest-0/test_complex_outputs_latex0/test_transform/test_complex_outputs_latex.obtained.xml
41
E       HTML DIFF: /tmp/pytest-of-runner/pytest-0/test_complex_outputs_latex0/test_transform/test_complex_outputs_latex.obtained.diff.html
42
E       --- 
43
E       +++ 
44
E       @@ -124,7 +124,6 @@
45
E                                plt.ylabel(r'a y label with latex $\alpha$')
46
E                                plt.legend();
47
E                        <CellOutputNode classes="cell_output">
48
E       -                    <image candidates="{'*': '_build/jupyter_execute/complex_outputs_17_0.png'}" uri="_build/jupyter_execute/complex_outputs_17_0.png">
49
E            <section ids="tables-with-pandas" names="tables\ (with\ pandas)">
50
E                <title>
51
E                    Tables (with pandas)

I tried updating this from svg to png as svg is removed from latex priorities list for mime types. But that didn't seem to work. png seem to be available in the notebook though.

[mmcky]

@chrisjsewell @choldgraf I don't see any docs on how to generate the xml representation to update the preferred solution for the latex test.

https://myst-nb.readthedocs.io/en/latest/develop/contributing.html#how-to-write-tests

I'll also submit a docs PR to document once I know.

[choldgraf]

Ah - so in this case, file_regression checks whether or not the data structure you give it differs from something on disk. If it differs at all then an error is raised.

If you want to regenerate the XML file on disk, just delete it, and re-run the tests. The test will fail the first time, saying the file doesn't exist, and it will create the file in the process. You should then inspect the file and make sure it looks correct. If so, re-run the test and it should pass.

[chrisjsewell]

If you want to regenerate the XML file on disk, just delete it, and re-run the tests.

Easier than that, just run pytest --force-regen. Does what its says, regenerates failing files

[choldgraf]

Easier than that, just run pytest --force-regen. Does what its says, regenerates failing files

:-O

[mmcky]

If you want to regenerate the XML file on disk, just delete it, and re-run the tests.

Easier than that, just run pytest --force-regen. Does what its says, regenerates failing files

Thanks guys. I can't seem to get it to work though. Do I need to issue this before the file is committed? OK Got it to work had to pip install pytest-regressions

> pytest --force-regen
ERROR: usage: pytest [options] [file_or_dir] [file_or_dir] [...]
pytest: error: unrecognized arguments: --force-regen
  inifile: /Users/matthewmckay/repos-collab/ebp/MyST-NB/pytest.ini
  rootdir: /Users/matthewmckay/repos-collab/ebp/MyST-NB

[mmcky]

@choldgraf it looks like the complex_outputs.ipynb file needs to be re-executed as the output block for the matplotlib figure is empty when I regenerate the xml representation. However the example.jpg file is missing. Do you know where I can find it to commit?

[choldgraf]

Is it an embedded file from markdown? Hmm weird. We could just point it to some other image file that's already in the repo

[mmcky]

Is it an embedded file from markdown? Hmm weird. We could just point it to some other image file that's already in the repo

Roger that. I'll just replace it -- seems only to be used in the one jupyter notebook.

[mmcky]

• @choldgraf when I re-executed the complex_outputs.ipynb and then used pytest --force-regen the xml for non-latex builds also updated. Would you mind to case a quick look at the diff. It looks ok to me with replacement binary blobs -- but some of the changes may be version specific such as pandas latex representations.

[choldgraf]

hmm - they look OK to me (though tests aren't passing). If the tests end up being brittle or platform dependent, then we can run the tests on a subset of the generated XML if need be

[chrisjsewell]

Btw, should we be entirely removing SVG from the priority list or just moving it to the bottom? This would be useful when the only output is SVG, in conjunction with sphinx.ext.imgconverter or sphinxcontrib-svg2pdfconverter.

[mmcky]

Btw, should we be entirely removing SVG from the priority list or just moving it to the bottom? This would be useful when the only output is SVG, in conjunction with sphinx.ext.imgconverter or sphinxcontrib-svg2pdfconverter.

@chrisjsewell I have removed it from latex. @choldgraf do you mind if I demote the svg output for html in this PR?

[chrisjsewell]

I have removed it from latex

Yes but that's the point; these extensions convert SVGs to PNG/PDF during the creation of latex outputs.

do you mind if I demote the svg output for html in this PR?

What would you be demoting it behind? SVG should most definitely be preferential to PNG/JPEG for HTML

[mmcky]

ah sorry @chrisjsewell I misunderstood your previous comment. I see what you're saying. Still learning how this mime stuff works. I'll add it to the end of latex. So sphinx can convert svg to png behind the scenes for latex? I was having issue with latex trying to \includegraphics[{{file.svg}] which won't work in LaTeX. svg was taking priority over png

[chrisjsewell]

So sphinx can convert svg to png behind the scenes for latex?

Yep that's the idea; they generally call on an external program like imagemagik, libRSVG or inkscape to convert them (which you obviously also need to have installed). Whether they are currently compatible with myst-nb is something I think should definitely be tested at some point; in terms of the stage in the sphinx process at which both are called. I know that I certainly use packages that only output SVG (like graphviz), so it would be good to still have a way to use these output when creating PDFs, and we should document the preferential way to do this somewhere in the documentation and/or in jupyterbook.

The main thing with graphics is that IMO, where possible, you should be looking to use vectorised (SVG/PDF) graphics, which have lossless quality with scaling, over rasterized (PNG/JPEG) ones.

[mmcky]

thanks @chrisjsewell -- that's helpful info. How do you trigger the conversion though. When svg was the preferred output the latex generated was including the svg as an image and then failing as \includegraphics couldn't parse the file type. Should latex be converting these through an update to preamble or should sphinx convert svg -> png prior to writing latex?

[chrisjsewell]

How do you trigger the conversion though

For SVG in standard rST files the conversion is just triggered by adding the extension to conf.py (sphinx.ext.imgconverter or sphinxcontrib-svg2pdfconverter).
Obviously without either of these extensions loaded, having only SVG will definitely fail at present.

The two sphinx extensions I mentioned do it on the sphinx end (prior to writing latex): this could be an issue, given that we inject the image into the doctree at quite a late stage (hence why I am not certain of compatibility). In the last few years there is now also a latex package which does similar on the latex end: https://ctan.org/pkg/svg?lang=en, which may be another option.

This should be raised as an issue to look into as a long-term goal

[chrisjsewell]

Looking over the code again; it looks like we "hard-code" in the priority lists:

MyST-NB/myst_nb/transform.py

Line 18 in afba1fe

RENDER_PRIORITY = {

As opposed to in jupyter-sphinx, where it is a configuration variable (albeit only allowing for a single list and not per builder): https://github.com/jupyter/jupyter-sphinx/blob/6a0df8575fd1fc76e1258aab300ec415eee92569/jupyter_sphinx/__init__.py#L155

I think the priority lists should be made configurable, to cope with use cases like this; whereby in general SVG should not be in the priority list for latex, since it will fail, but then you may want to add it in if you are also using an extension that makes it compatible.

[mmcky]

thanks @chrisjsewell I agree with configurability. Looking at sphinxcontrib-svg2pdfconverter seems like a good option to support svg so I will open an issue to track this discussion for future support.

one step ahead I see - thanks!

[chrisjsewell]

Just opened an issue 👍

[mmcky]

Regenerating the support test files is failing using pytest --force-regen. It seems like there is a mismatch in the set of files for test test_complex_output but don't understand how to debug the issue.

>       assert filenames == {
            "complex_outputs_17_0.svg",
            "complex_outputs.ipynb",
            "complex_outputs_17_0.pdf",
            "complex_outputs.py",
            "complex_outputs_24_0.png",
            "complex_outputs_13_0.jpg",
        }
E       AssertionError: assert {'complex_out...uts_24_0.png'} == {'complex_out...uts_24_0.png'}
E         Extra items in the left set:
E         'complex_outputs_17_0.png'
E         Extra items in the right set:
E         'complex_outputs_17_0.pdf'
E         'complex_outputs_17_0.svg'
E         Use -v to get the full diff

/repos-collab/ebp/MyST-NB/tests/test_parser.py:61: AssertionError
---------------------------------------------------------------- Captured stdout call -----------------------------------------------------------------
{'complex_outputs.ipynb', 'complex_outputs_24_0.png', 'complex_outputs_17_0.png', 'complex_outputs.py', 'complex_outputs_13_0.jpg'}
=============================================================== short test summary info ===============================================================
FAILED tests/test_parser.py::test_complex_outputs - AssertionError: assert {'complex_out...uts_24_0.png'} == {'complex_out...uts_24_0.png'}
============================================================ 1 failed, 34 passed in 21.13s ============================================================

[chrisjsewell]

but don't understand how to debug the issue.

Before we were getting pdf and svg outputs from cell 17, now we are only getting png.
I imagine that is the matplotlib outputs. I don't see why that has only just changed in this PR, but I known that in a previous commit, I removed the ipypublish code from the test notebooks, which was calling this function, to set up matplotlib to output PDF and SVG and also another to set up pandas to output latex: https://github.com/chrisjsewell/ipypublish/blob/43c6ff4be0b8eac3c7d2062cec6d1e459d5baa0c/ipypublish/scripts/nb_setup.py#L177.

They're actually very useful functions for getting these packages to create optimal outputs for sphinx, and would be good if we could incorporate them somewhere in the executablebook stack

[mmcky]

I don't see why that has only just changed in this PR

Agreed. Don't understand why the change is in this PR from looking at what has changed.

[mmcky]

@choldgraf @chrisjsewell I made this change to the test_parser set of tests but not 100% sure why it needed to be made.

For some reason we are generating the png file only for complex_.._17 and not the svg and pdf versions of that image.

Tests should pass now.

[codecov]

Codecov Report

Merging #176 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #176   +/-   ##
=======================================
  Coverage   84.26%   84.26%           
=======================================
  Files           9        9           
  Lines         820      820           
=======================================
  Hits          691      691           
  Misses        129      129           

Flag	Coverage Δ
#pytests	84.26% <ø> (ø)

Impacted Files	Coverage Δ
myst_nb/transform.py	75.67% <ø> (ø)

---

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 afba1fe...291f0d6. Read the comment docs.

[mmcky]

• this fixes latexpdf builds for https://github.com/executablebooks/myst-nb.example-project

[choldgraf]

LGTM - and you all are the latex experts so I will defer to your thinking on the best way to get it working. Are we ready to merge?

[choldgraf]

ping for @mmcky - it this is all ready to go then let's get it in, just need to hear from y'all if that's the case!

[mmcky]

sorry @choldgraf -- didn't see first message -- good to go.

[mmcky]

once this is merged I will test more widely

[choldgraf]

ok let's do it! :-D