Ticket #5433 (closed defect: fixed)
[with patch, positive review] LaTeX fixes
| Reported by: | jhpalmieri | Owned by: | jhpalmieri |
|---|---|---|---|
| Priority: | minor | Milestone: | sage-3.4.1 |
| Component: | misc | Keywords: | |
| Cc: | Work issues: | ||
| Report Upstream: | Reviewers: | ||
| Authors: | Merged in: | ||
| Dependencies: | Stopgaps: |
Description
The attached patch fixes some _latex_ methods: things like "{\rm blah}" have been changed to "\mathrm{blah}", and things like "\mbox{\bf blah}" have been changed to "\mathbf{blah}".
Attachments
Change History
comment:1 follow-up: ↓ 2 Changed 4 years ago by jhpalmieri
Oh, right, it also does something else: it changes the _latex_file_ function in sage/misc/latex.py: I changed the default arguments, added documentation, and removed the "brk" argument since I really couldn't see the point: setting brk equal to 2 would change \begin{document} blah to \b eg in {d oc um en t} bl ah. Why?
As far as changing the default values, we made many of the same changes in the view function a while ago: see #3145.
I made corresponding changes to the png function in that file.
comment:2 in reply to: ↑ 1 ; follow-up: ↓ 3 Changed 4 years ago by rbeezer
- Summary changed from [with patch, needs review] LaTeX fixes to [with patch, review pending] LaTeX fixes
Replying to jhpalmieri:
Hi John,
Working on a review. Looks good and all tests passed.
There was a time when TeX would choke on long input (more than 256 characters?) so maybe that explains the use of "brk"? But I think those days are behind us, so it is probably safe to drop it.
Two suggestions:
- In the docstring for _latex_file_() it is not real clear that the sep argument is for decoration rather than function. Maybe additional explanation like: "for example, sep='\\hrule'", would make it clearer that this is not a LaTeX requirement like "$$" (and it also needs an escaped backslash for any TeX commands). (Which I now see is discussed in #3145.)
- Are all the "empty" \pagestyle and \thispagestyle really needed? There is one present in the header, but then two show up between each object's latex representation. They don't seem to be hurting anything, but maybe now would be a good time to have them go away, perhaps along with a \n or two.
Rob
comment:3 in reply to: ↑ 2 ; follow-up: ↓ 4 Changed 4 years ago by jhpalmieri
Replying to rbeezer:
Hi Rob,
Thanks for your comments. I'm attaching a new patch (to be applied on top of the other one) addressing them, plus a bit more:
I've expanded the docstring for _latex_file considerably, including comments about sep, and I've removed a bunch of the \pagestyle (etc.) commands.
I also changed the default spacing in the LaTeX file: it used to be
title (centered) \vfill object \vfill
and I've changed it to
title (centered)
\vspace{40mm}
object
I think this looks better, but we can change it back if you disagree.
It used to be that if tiny=False, then the file contained "
small", and I've deleted this.
Finally, view and _latex_file had options to pass on to xdvi: 'expert' and 'zoom'. These were not used anywhere in the code, and in fact I don't see how you even could pass them as arguments to xdvi, given the changes in #3137. So I've deleted those options.
John
comment:4 in reply to: ↑ 3 Changed 4 years ago by rbeezer
- Summary changed from [with patch, review pending] LaTeX fixes to [with patch, positive review] LaTeX fixes
Replying to jhpalmieri:
Hi John,
Looks great - lots of good documentation and code clean-up, plus much more readable LaTeX source being produced by _latex_file_(), in addition to the typeset result being improved.
latex.py passed tests and performs as advertised. view() works at command line through xdvi or in the notebook.
And that's the most entertaining docstring I've seen yet. ;-)
Positive review. Apply both patches in the order attached here.
Rob

