Ticket #5433 (closed defect: fixed)

Opened 4 years ago

Last modified 4 years ago

[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

latex.patch Download (16.4 KB) - added by jhpalmieri 4 years ago.
latex-delta.2.patch Download (7.7 KB) - added by jhpalmieri 4 years ago.
apply on top of other patch

Change History

Changed 4 years ago by jhpalmieri

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:

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

Changed 4 years ago by jhpalmieri

apply on top of other patch

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

comment:5 Changed 4 years ago by mabshoff

  • Status changed from new to closed
  • Resolution set to fixed
  • Milestone changed from sage-3.4.2 to sage-3.4.1

Merged both patches in Sage 3.4.1.alpha0.

Cheers,

Michael

Note: See TracTickets for help on using tickets.