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:  sage4.8 
Component:  misc  Keywords:  sd35 
Cc:  tdemedts@…, jhpalmieri  Merged in:  sage4.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 )
An object of type LatexExpr
should be prettyprinted asis, 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)
Change History (36)
comment:1 Changed 9 years ago by
 Description modified (diff)
comment:2 Changed 9 years ago by
 Cc tdemedts@… added
Changed 9 years ago by
comment:3 Changed 9 years ago by
 Status changed from new to needs_review
comment:4 Changed 9 years ago by
 Cc jhpalmieri added
Changed 9 years ago by
comment:5 Changed 9 years ago by
 Reviewers set to Andrey Novoseltsev
comment:6 Changed 9 years ago by
 Description modified (diff)
comment:7 Changed 9 years ago by
I removed "Strings are wrapped into verbatim environment for typeset output, while LaTeX expressions are left asis" because I think that's confusing. I added some more documentation changes and examples. Needs review.
comment:8 Changed 9 years ago by
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 followup: ↓ 10 Changed 9 years ago by
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 ; followup: ↓ 12 Changed 9 years ago by
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
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
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 alignenvironment in a paper. In editors that autowrap 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 followup: ↓ 14 Changed 9 years ago by
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
 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
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
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.
comment:17 Changed 9 years ago by
 Description modified (diff)
 Work issues _add_() deleted
Changed 9 years ago by
comment:18 Changed 9 years ago by
 Status changed from needs_work to needs_review
Positive review to everything except my extra_doc patch.
comment:19 Changed 9 years ago by
 Description modified (diff)
comment:20 Changed 9 years ago by
 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
 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
 Work issues set to sd35
comment:23 Changed 9 years ago by
 Keywords sd35 added
 Work issues sd35 deleted
comment:24 Changed 9 years ago by
 Dependencies set to #12197
 Status changed from needs_work to needs_review
comment:25 Changed 9 years ago by
 Description modified (diff)
Changed 9 years ago by
comment:26 Changed 9 years ago by
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
 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
comment:28 Changed 9 years ago by
 Status changed from needs_review to positive_review
Reviewer patch looks fine.
comment:29 Changed 9 years ago by
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 511 511 for i in range(len(self.values_on_gens())): 512 512 if i != 0: 513 513 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_() 515 515 return r'Dirichlet character modulo %s of conductor %s mapping $%s$'%(self.modulus(), self.conductor(), mapst) 516 516 517 517 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
 Merged in set to sage4.8.alpha6
 Resolution set to fixed
 Status changed from positive_review to closed
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.