Opened 9 years ago

Closed 9 years ago

#12156 closed defect (fixed)

Pretty print LatexExpr directly

Reported by: jdemeyer Owned by: jason
Priority: major Milestone: sage-4.8
Component: misc Keywords: sd35
Cc: tdemedts@…, jhpalmieri Merged in: sage-4.8.alpha6
Authors: Jeroen Demeyer, Punarbasu Purkayastha Reviewers: Andrey Novoseltsev, Punarbasu Purkayastha, John Palmieri
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: #12197 Stopgaps:

Description (last modified by jhpalmieri)

An object of type LatexExpr should be pretty-printed as-is, that is

sage: pretty_print(LatexExpr('x^2 + 1'))

should really typeset "x^2 + 1" instead of displaying the string "x^2 + 1".

Also, import LatexExpr globally.

Apply 12156_latex_pretty_print.patch, 12156_reviewer.patch, 12156_add_LatexExpr.patch, 12156_extra_doc.patch, 12156_fix_latex.patch, 12156_jhp.patch

Attachments (6)

12156_latex_pretty_print.patch (5.5 KB) - added by jdemeyer 9 years ago.
12156_reviewer.patch (1.7 KB) - added by novoselt 9 years ago.
12156_add_LatexExpr.patch (844 bytes) - added by jdemeyer 9 years ago.
Add to LatexExpr? (added space in between the two)
12156_extra_doc.patch (2.9 KB) - added by jdemeyer 9 years ago.
12156_fix_latex.patch (10.7 KB) - added by jdemeyer 9 years ago.
12156_jhp.patch (1.7 KB) - added by jhpalmieri 9 years ago.

Download all attachments as: .zip

Change History (36)

comment:1 Changed 9 years ago by jdemeyer

  • Description modified (diff)

comment:2 Changed 9 years ago by jdemeyer

  • Authors set to Jeroen Demeyer
  • Cc tdemedts@… added

Changed 9 years ago by jdemeyer

comment:3 Changed 9 years ago by jdemeyer

  • Status changed from new to needs_review

comment:4 Changed 9 years ago by jhpalmieri

  • Cc jhpalmieri added

Changed 9 years ago by novoselt

comment:5 Changed 9 years ago by novoselt

  • Reviewers set to Andrey Novoseltsev

I got a bit confused by the description of the class, so I propose the attached change. The main patch has positive review from me.

comment:6 Changed 9 years ago by jdemeyer

  • Description modified (diff)

comment:7 Changed 9 years ago by jdemeyer

I removed "Strings are wrapped into verbatim environment for typeset output, while LaTeX expressions are left as-is" because I think that's confusing. I added some more documentation changes and examples. Needs review.

comment:8 Changed 9 years ago by ppurka

There is just one point. It is not possible to combine normal strings with latex strings or LatexExpr? with itself. Something like this fails to produce the right output: LatexExpr(r'\Delta') + LatexExpr(r'\frac{x}2'). This gives the wrong output even though the addition goes through as strings. Proposed patch is attached, though I leave it up to you to decide whether to include it.

Other than this minor point, the patches provided earlier work as advertised. So, positive review from my side.

comment:9 follow-up: Changed 9 years ago by novoselt

If we do add the last patch (which is a bit beyond the title of this ticket), then I think that we MUST add a space between concatenated expressions: if you join "x" and "y" into "xy" it may be exactly what you need, but "\alpha" and "x" get joined into "\alphax" which is not a valid code any more (as I have "discovered" at #9478). Plus spaces (or even new lines) help reading the code and adjusting it if necessary.

I also was actually surprised that "\frac{x}2" works, I would put it as "\frac{x}{2}", although it is just a personal preference, I guess ;-)

comment:10 in reply to: ↑ 9 ; follow-up: Changed 9 years ago by jdemeyer

Replying to novoselt:

If we do add the last patch (which is a bit beyond the title of this ticket), then I think that we MUST add a space between concatenated expressions: if you join "x" and "y" into "xy" it may be exactly what you need, but "\alpha" and "x" get joined into "\alphax" which is not a valid code any more (as I have "discovered" at #9478). Plus spaces (or even new lines) help reading the code and adjusting it if necessary.

The problem is that maybe sometimes you don't want a space. How about adding a "{}" instead which acts like a seperator but otherwise doesn't do much.

comment:11 Changed 9 years ago by jdemeyer

This breaks the pdf manual:

Runaway argument?
\PYG {g+gp}{sage: }\PYG {n}{LatexExpr}\PYG {p}{(}\PYG {l+s}{r"}\PYG {\ETC.
! Forbidden control sequence found while scanning use of \FancyVerbGetLine.
<inserted text>
                \par
l.126604 ...r}\PYG{p}{(}\PYG{l+s}{r"}\PYG{l+s}{^^L
                                                  rac\PYGZob{}x\PYGZca{}2 + ...

?
! Emergency stop.
<inserted text>
                \par
l.126604 ...r}\PYG{p}{(}\PYG{l+s}{r"}\PYG{l+s}{^^L
                                                  rac\PYGZob{}x\PYGZca{}2 + ...

!  ==> Fatal error occurred, no output PDF file produced!
Transcript written on reference.log.

comment:12 in reply to: ↑ 10 Changed 9 years ago by novoselt

Replying to jdemeyer:

The problem is that maybe sometimes you don't want a space. How about adding a "{}" instead which acts like a seperator but otherwise doesn't do much.

In what situations does the space matter in math mode? It seems to me that if both were valid expressions before, then they can be separated by a space without any harm... As I understand {} will do the same, but it will make the code much uglier, x{}y is not as cool as x y. And I think that readability of the LaTeX output is a valid concern since I quite regularly take some Sage generated code and then do some manual tweaking. For example - take a long polynomial and then typeset it in an align-environment in a paper. In editors that auto-wrap lines on spaces it is also preferable to have just a space separator.

If there are situations when space does not work, which I think are quite rare since I cannot think of any, one always has an option of converting things to strings, doing anything there and then assembling them back. On a related note, it would be nice to have automatic vertical assembling of such expressions as well, but it is definitely beyond this ticket.

comment:13 follow-up: Changed 9 years ago by novoselt

Which patch does break the PDF manual? The last one or all of them?

comment:14 in reply to: ↑ 13 Changed 9 years ago by jdemeyer

  • Status changed from needs_review to needs_work
  • Work issues set to _add_()

Replying to novoselt:

Which patch does break the PDF manual? The last one or all of them?

Problem solved by using r""" instead of """.

Concerning adding: you are right, I did not consider that we are using math mode all the time. But you should use _add_ (single underscores). I don't know why, but Sage does it this way.

comment:15 Changed 9 years ago by novoselt

I think _add_ is used so that coercion can work properly. But it requires inheriting from SageObject or maybe even Element, so that generic __add__ can do its part of the job. So I doubt it will work for LatexExpr which inherits from str and nothing else.

comment:16 Changed 9 years ago by ppurka

Actually space seems to work just fine to separate elements. Since we are in math mode, JSMath will collapse the extra spaces automatically.

As mentioned by novoselt, _add_ does not work. I tried it out just now. Updated the patch.

Changed 9 years ago by jdemeyer

Add to LatexExpr? (added space in between the two)

comment:17 Changed 9 years ago by jdemeyer

  • Description modified (diff)
  • Work issues _add_() deleted

Changed 9 years ago by jdemeyer

comment:18 Changed 9 years ago by jdemeyer

  • Status changed from needs_work to needs_review

Positive review to everything except my extra_doc patch.

comment:19 Changed 9 years ago by jdemeyer

  • Description modified (diff)

comment:20 Changed 9 years ago by novoselt

  • Authors changed from Jeroen Demeyer to Jeroen Demeyer, Punarbasu Purkayastha
  • Reviewers changed from Andrey Novoseltsev to Andrey Novoseltsev, Punarbasu Purkayastha
  • Status changed from needs_review to positive_review

Positive review to the last patch as well! Why do we have the reviewer field before the author one?..

comment:21 Changed 9 years ago by jdemeyer

  • Status changed from positive_review to needs_work

Unfortunately there are some doctest errors in various unrelated files. I am working on a patch.

comment:22 Changed 9 years ago by jdemeyer

  • Work issues set to sd35

comment:23 Changed 9 years ago by jdemeyer

  • Keywords sd35 added
  • Work issues sd35 deleted

comment:24 Changed 9 years ago by jdemeyer

  • Dependencies set to #12197
  • Status changed from needs_work to needs_review

comment:25 Changed 9 years ago by jdemeyer

  • Description modified (diff)

Changed 9 years ago by jdemeyer

comment:26 Changed 9 years ago by jdemeyer

New patch also allows str + LatexExpr? additions, which needed a few changes in various places inside Sage. I left one more substantial change for differential forms at #12197.

Needs review.

comment:27 Changed 9 years ago by jhpalmieri

  • Description modified (diff)
  • Reviewers changed from Andrey Novoseltsev, Punarbasu Purkayastha to Andrey Novoseltsev, Punarbasu Purkayastha, John Palmieri

I think I'll join the patch party: I'm attaching a patch which rewords a few docstrings. Otherwise, everything looks good to me. With the patch at #12197 and all of the patches here, all tests pass.

I'm willing to give everything but my patch a positive review.

Changed 9 years ago by jhpalmieri

comment:28 Changed 9 years ago by jdemeyer

  • Status changed from needs_review to positive_review

Reviewer patch looks fine.

comment:29 Changed 9 years ago by ppurka

I guess you are patching some 4.8alpha version of sage. I was patching 4.7.2, so I had to modify the patch 12156_fix_latex.patch for sage/modular/dirichlet.py to read as:

  • sage/modular/dirichlet.py

    diff --git a/sage/modular/dirichlet.py b/sage/modular/dirichlet.py
    a b  
    511511        for i in range(len(self.values_on_gens())):
    512512            if i != 0:
    513513                mapst += ', '
    514             mapst += latex(self.parent().unit_gens()[i]) + ' \mapsto ' + latex(self.values_on_gens()[i])
     514            mapst += self.parent().unit_gens()[i]._latex_() + ' \mapsto ' + self.values_on_gens()[i]._latex_()
    515515        return r'Dirichlet character modulo %s of conductor %s mapping $%s$'%(self.modulus(), self.conductor(), mapst)
    516516                 
    517517        def base_ring(self):

This, along with #12197 makes it pass the doctests. So, positive review from me too.

comment:30 Changed 9 years ago by jdemeyer

  • Merged in set to sage-4.8.alpha6
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.