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

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

Open
lezcano opened this issue Jun 28, 2021 · 13 comments
Open

Comments

@lezcano
Copy link

lezcano commented Jun 28, 2021

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

@mattip
Copy link
Contributor

mattip commented Jun 28, 2021

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?

@lezcano
Copy link
Author

lezcano commented Jun 29, 2021

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
Copy link
Author

lezcano commented Jun 29, 2021

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.

@mattip mattip mentioned this issue Aug 1, 2021
@mattip
Copy link
Contributor

mattip commented Aug 3, 2021

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

@mattip
Copy link
Contributor

mattip commented Aug 3, 2021

Digging into this, it seems PR #81 changed the background color in this segment of code

article.pytorch-article .section :not(dt) > code {
color: #262626;
border-top: solid 2px #ffffff;
background-color: #ffffff;
border-bottom: solid 2px #ffffff;
padding: 0px 3px;
-webkit-box-decoration-break: clone;
box-decoration-break: clone;
}

Previously it was

article.pytorch-article .section :not(dt) > code {
color: #262626;
border-top: solid 2px #f3f4f7;
background-color: #f3f4f7;
border-bottom: solid 2px #f3f4f7;
padding: 0px 3px;
-webkit-box-decoration-break: clone;
box-decoration-break: clone;
}

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
Copy link
Contributor

mattip commented Aug 20, 2021

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
Copy link
Contributor

mattip commented Aug 20, 2021

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

@lezcano lezcano changed the title Spaces within double backticks are not rendered correctly. The background of spaces within double backticks are not rendered correctly. Aug 20, 2021
@lezcano
Copy link
Author

lezcano commented Aug 20, 2021

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).

@mattip
Copy link
Contributor

mattip commented Aug 20, 2021

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>

@lezcano
Copy link
Author

lezcano commented Aug 20, 2021

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?

@mattip
Copy link
Contributor

mattip commented Aug 21, 2021

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

@adamjstewart
Copy link

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
Copy link

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

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

No branches or pull requests

3 participants