Opened 4 years ago

Closed 4 years ago

#17745 closed defect (fixed)

typo causes latex error in indexed generators

Reported by: kcrisman Owned by:
Priority: minor Milestone: sage-6.5
Component: documentation Keywords:
Cc: tscrim, nthiery Merged in:
Authors: John Palmieri Reviewers: Travis Scrimshaw
Report Upstream: N/A Work issues:
Branch: 0065db9 (Commits) Commit: 0065db912af0bebb7f1149089e3a1b5b7159cd97
Dependencies: Stopgaps:

Description

See http://ask.sagemath.org/question/25763/incorrect-parsing-of-docstring-of-sagestructureindexed_generatorsindexedgenerators/

where a \left is displayed as <=ft. It seems that \le in \left gets replaced by <=.

This is from #15289.

Change History (26)

comment:1 Changed 4 years ago by tscrim

I believe this is a general issue with the parsing of latex docstrings to a more ascii version just being too greedy (because I saw it somewhere else IIRC). Do you happen to know where the code that does this substitution is?

comment:2 Changed 4 years ago by aapitzsch

Problem seems to be in src/sage/misc/sagedoc.py

math_substitutes = [
    ...
    ('\\le', '<='),
    ...
]

comment:3 Changed 4 years ago by aapitzsch

One solution might be to replace '\\left' and '\\right' by '' before replacing \\le.

comment:4 Changed 4 years ago by tscrim

I agree, along with perhaps:

  • '\\lvert' and '\\rvert' by '|',
  • '\\ast' by *,
  • '\\bigr', '\\bigl', etc. by ''.

comment:5 Changed 4 years ago by jhpalmieri

  • Branch set to u/jhpalmieri/typo_causes_latex_error_in_indexed_generators

comment:6 Changed 4 years ago by jhpalmieri

  • Commit set to 5792ca9ca3dbc21bd688e288a766c8563874febd

Here's an attempt at a fix, along the lines suggested.


New commits:

5792ca9#17745: remove \\left and \\right when running detex

comment:7 Changed 4 years ago by jhpalmieri

  • Authors set to John Palmieri
  • Status changed from new to needs_review

comment:8 Changed 4 years ago by tscrim

  • Reviewers set to Travis Scrimshaw

LGTM, but could you add one more test with a \\left, a \\le, and a \\leq to make sure all 3 get converted correctly? Thanks.

Last edited 4 years ago by tscrim (previous) (diff)

comment:9 Changed 4 years ago by git

  • Commit changed from 5792ca9ca3dbc21bd688e288a766c8563874febd to ef864a4e87afbf9776d811fc1333809194ee4b55

Branch pushed to git repo; I updated commit sha1. New commits:

ef864a417745: one more doctest

comment:10 Changed 4 years ago by jhpalmieri

Sure, here you go. Eventually, we might run into the same problem with \\to or \\ge; at some point we might want to change the whole system so it uses regular expressions instead of just a plain text replace.


New commits:

ef864a417745: one more doctest

comment:11 Changed 4 years ago by tscrim

  • Status changed from needs_review to positive_review

Thanks. Probably, but we'll cross that bridge if we come to it.

comment:12 Changed 4 years ago by jhpalmieri

  • Status changed from positive_review to needs_work

I realized that this won't work: it will turn \\leftarrow and \\rightarrow into arrow. I think regular expressions are the way to go.

comment:13 Changed 4 years ago by tscrim

How about then just putting \\leftarrow and \\rightarrow before \left and \right (sending them to <-- and --> respectively?

Last edited 4 years ago by tscrim (previous) (diff)

comment:14 Changed 4 years ago by jhpalmieri

Because I don't think we should actually be trying to run a serious LaTeX-to-text conversion here, and that's what this is in danger of becoming. What about \leftrightarrow, \leftleftarrows, etc.?

comment:15 Changed 4 years ago by tscrim

I'm not so familiar with using regular expressions, so if you can make it more elegant that way, then +1 from me.

comment:16 Changed 4 years ago by git

  • Commit changed from ef864a4e87afbf9776d811fc1333809194ee4b55 to 18c6a2771996c623276e9cc362c9eecda69e4c4e

Branch pushed to git repo; I updated commit sha1. New commits:

18c6a2717745: use regular expressions

comment:17 Changed 4 years ago by jhpalmieri

Here's a regular expression version.

comment:18 Changed 4 years ago by jhpalmieri

  • Status changed from needs_work to needs_review

comment:19 follow-up: Changed 4 years ago by aapitzsch

The current implementation replaces LaTeX commands in the examples section, too. See for example

sage: from sage.structure.indexed_generators import IndexedGenerators
sage: IndexedGenerators?

That's bad. Users might complain, that they don't get the results show in the examples.

P.S. The ticket description should be updated.

comment:20 in reply to: ↑ 19 Changed 4 years ago by jhpalmieri

Replying to aapitzsch:

The current implementation replaces LaTeX commands in the examples section, too.

I think that's complicated to fix. We would need to detect when we're in an EXAMPLES or TESTS block and ignore the output, but otherwise process the output. I guess we can do that, but it's a little annoying to do perfectly. This really should be done for each of detex, process_dollars, process_extlinks, process_mathtt. So maybe it should actually be done in format:

if not in examples block:
    detex(next line)
    process_dollars(next_lines)
    ...

So everything would need to be rewritten with this logic in mind. Alternatively, each of those functions will duplicate some code which keeps track of whether it's processing a line in an examples block.

I think that really, functions which have LaTeX output in their EXAMPLES (or elsewhere in their docstrings), and if that LaTeX should not be processed, then they should have their docstrings decorated with nodetex. That's why nodetex is available. Maybe we should add it to the IndexedGenerators docstring.

P.S. The ticket description should be updated.

Please go ahead. What did you have in mind?

comment:21 Changed 4 years ago by git

  • Commit changed from 18c6a2771996c623276e9cc362c9eecda69e4c4e to c413a14653f72e20eb6553459d002f97f6280551

Branch pushed to git repo; I updated commit sha1. New commits:

c413a1417745: add nodetex to IndexedGenerators docstring

comment:22 Changed 4 years ago by kcrisman

Probably this will also fix the followup comment at the ask.sagemath question:

sage: timeit("a = 2nb=131nfactor(a^b-1)", number=25)
  25 loops, best of 3: ... per loop

comment:23 Changed 4 years ago by git

  • Commit changed from c413a14653f72e20eb6553459d002f97f6280551 to 0065db912af0bebb7f1149089e3a1b5b7159cd97

Branch pushed to git repo; I updated commit sha1. New commits:

c67dd3dMerge branch 'develop' into t/17745/typo_causes_latex_error_in_indexed_generators
0065db917745: add more instances of nodetex

comment:24 Changed 4 years ago by kcrisman

And even more examples from the same place:

Another such issues appear for dirac_delta, heaviside, unit_step. A \left gets replaced by <=ft

comment:25 Changed 4 years ago by tscrim

  • Status changed from needs_review to positive_review

I think this is a good step in the right direction; most importantly, it fixes the issue at hand. So I'm going to give this a positive review.

comment:26 Changed 4 years ago by vbraun

  • Branch changed from u/jhpalmieri/typo_causes_latex_error_in_indexed_generators to 0065db912af0bebb7f1149089e3a1b5b7159cd97
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.