pytorch_sphinx_theme icon indicating copy to clipboard operation
pytorch_sphinx_theme copied to clipboard

The background of spaces within double backticks are not rendered correctly.

Open lezcano opened this issue 3 years ago • 13 comments

When one writes code with spaces like ``input = L @ L.T`` this is rendered in a rather awkward as image

Interestingly enough, this problem does not happen when we write that code in a non-white background, like that from .. note:: or .. warning::. See the first warning in https://pytorch.org/docs/1.9.0/generated/torch.svd.html

We tentatively solved this in torch.linalg by preferring single backticks over double backticks, but when referring to inputs of the function, that we used :attr:`input`. This has its own problems, as code as the one in the example above has to be written in a rather awkward way: :attr:`input`\ `= L @ L.T`. Things become more difficult when there are several attributes in the expression.

It would be convenient to solve the problem with the rendering of the inline code with two backticks in the template to be able to always prefer it over single backticks and the attr construction.

cc @mattip

lezcano avatar Jun 28 '21 15:06 lezcano

Using single backticks is bad form. They are used to indicate a link, not for code formatting. Could you give an example of the awkward formatting?

mattip avatar Jun 28 '21 17:06 mattip

Sorry, I thought I had left a link.

See for example the rendering of (U, S, Vh) in the first paragraph of https://pytorch.org/docs/1.8.0/linalg.html?highlight=svd#torch.linalg.svd Interestingly enough, this construction renders correctly in the docs of torch.svd of PyTorch 1.7.0 https://pytorch.org/docs/1.7.0/generated/torch.svd.html

lezcano avatar Jun 29 '21 10:06 lezcano

Upon further investiagation, the code in 1.7.0 was also ``(U, S, V)``: https://github.com/pytorch/pytorch/blame/006cfebf3dcc4bc0cba344c25243baebaeeefbb7/torch/_torch_docs.py#L8145 so it is not clear to me what's going on here.

lezcano avatar Jun 29 '21 10:06 lezcano

How does the latest nightly build look after #135? https://pytorch.org/docs/master/generated/torch.linalg.svd.html

mattip avatar Aug 03 '21 10:08 mattip

Digging into this, it seems PR #81 changed the background color in this segment of code https://github.com/pytorch/pytorch_sphinx_theme/blob/114bdb63087ad884a6b3347d6773b369496b7c9b/pytorch_sphinx_theme/static/css/theme.css#L10566-L10574

Previously it was https://github.com/pytorch/pytorch_sphinx_theme/blob/f04a90e511c62ca98a9f2d076a2230772f446ee6/pytorch_sphinx_theme/static/css/theme.css#L10522-L10530

It seems #135 did the wrong thing and probably should be backed out. I am not sure how actually to fix this and how the scss interfaces with the theme.css file, since the theme.css is checked into git but the CI job seems to regenerate it? @brianjo could you give some guidance?

mattip avatar Aug 03 '21 11:08 mattip

I think there is a real mess with the desire to set a different background color for elements. The background should be an attribute of the surrounding <span> or <div>, and not of the actual text. I removed some of the background formatting in #138, but the top warning on this page still has weird background remnants around the text. Should I go further in removing the background colors?

@brianjo

mattip avatar Aug 20 '21 12:08 mattip

@Lezcano could you change the title to indicate that the problem is specifically the background color?

mattip avatar Aug 20 '21 12:08 mattip

Fixed the title.

Also, to help testing whether this is fixed, here are two examples of the problem.

In: https://pytorch.org/docs/master/generated/torch.bincount.html we have the formula out[n] += weights[i] with a non-uniform background.

In https://pytorch.org/docs/master/generated/torch.qr.html in the warning, the formulae with double backticks (e.g. Q, R = torch.qr(A)) have a background around each letter on a dark grey (which is not particularly nice) and then a lighter background around the whole formula (which is what I think we would want).

lezcano avatar Aug 20 '21 12:08 lezcano

a lighter background around the whole formula

Q, R = torch.qr(A) is rendered as <code class="docutils literal notranslate"><span class="pre">Q,</span> <span class="pre">R</span> <span class="pre">=</span> <span class="pre">torch.qr(A)</span></code>.

There is no specific formatting for the "literal" nor the "notranslate" classes. I think the path forward would be to define a style for "notranslate" with a background color. This would also cover the code sample with the linalg namespace Q, R = torch.linalg.qr(A). I guess the different span classses are n for name, o for operator and p for punctuation. Do theme creators actually want that level of customation?

<div class="highlight-python notranslate"><div class="highlight"><pre id="codecell0">
    <span class="n">Q</span><span class="p">,</span> <span class="n">R</span>
    <span class="o">=</span> <span class="n">torch</span><span class="o">.</span>
    <span class="n">linalg</span><span class="o">.</span><span class="n">qr</span>
    <span class="p">(</span><span class="n">A</span>
    <span class="p">)</span>
</pre></div>
</div>

mattip avatar Aug 20 '21 13:08 mattip

Would that remove all these <span class="pre">? I reckon that all those are not necessary.

Also, note that the background color would need to be different for the two backgrounds that we have, white (default) and grey (in notes and warnings). I think it'd be particularly nice if we used the grey as a background for notranslate when not in a note / warning and white when in a note / warning. Would this be possible?

lezcano avatar Aug 20 '21 14:08 lezcano

Would that remove all these ? I reckon that all those are not necessary.

Those come from the way sphinx parses a function signature, and I do not see an easy way to fold those together. The relevant code is in _parse_arglist, and the nodes are desc_sig_name (for the n), desc_sig_operator (for the o) and desc_sig_punctuation (for the p)

Also, note that the background color would need to be different for the two backgrounds that we have, white (default) and grey (in notes and warnings). I think it'd be particularly nice if we used the grey as a background for notranslate when not in a note / warning and white when in a note / warning. Would this be possible?

Sounds reasonable

mattip avatar Aug 21 '21 20:08 mattip

Just hit this as well. See spack info py-torchgeo at https://torchgeo.readthedocs.io/en/latest/user/installation.html for another example. Would love to see this fixed, but I don't know CSS well enough to help.

adamjstewart avatar Sep 22 '21 15:09 adamjstewart

This seems to be a duplicate of #107, we should close one or the other.

adamjstewart avatar Sep 22 '21 15:09 adamjstewart