Opened 12 years ago
Closed 11 years ago
#11311 closed defect (fixed)
engine="pdflatex" in view is ignored
Reported by:  Franco Saliola  Owned by:  jason, was 

Priority:  major  Milestone:  sage5.1 
Component:  graphics  Keywords:  latex, pdflatex, days30 
Cc:  John Palmieri  Merged in:  sage5.1.beta4 
Authors:  Franco Saliola, John Palmieri  Reviewers:  KarlDieter Crisman, Franco Saliola, John Palmieri 
Report Upstream:  N/A  Work issues:  
Branch:  Commit:  
Dependencies:  Stopgaps: 
Description (last modified by )
The view command states that the pdflatex argument is deprecated and that one should use the engine argument instead.
But if one sets engine="pdflatex", it is overwritten, and so one currently needs to use the deprecated pdflatex option.
Apply trac_113115.0.patch.
Attachments (6)
Change History (43)
Changed 12 years ago by
Attachment:  trac_11311_engine_pdflatex_viewfs.patch added 

comment:1 Changed 12 years ago by
Status:  new → needs_review 

comment:2 Changed 12 years ago by
Cc:  John Palmieri added 

comment:3 Changed 12 years ago by
Keywords:  days30 added 

comment:4 Changed 12 years ago by
Authors:  → Franco Saliola 

comment:5 Changed 12 years ago by
Changed 12 years ago by
Attachment:  trac_11311_engine_pdflatex_viewfs.2.patch added 

comment:6 Changed 12 years ago by
Description:  modified (diff) 

Thanks for catching this. I corrected this in the new patch.
comment:7 Changed 12 years ago by
Description:  modified (diff) 

comment:8 Changed 12 years ago by
Franco, you need to rebase this against your own patch at #11315 which was recently merged :)
I may do this if the patch behaves as promised.
comment:9 Changed 12 years ago by
Reviewers:  → KarlDieter Crisman 

Status:  needs_review → needs_info 
Question: when I do
sage: view(2,engine='pdflatex')
it opens in my pdf viewer, not in my TeX program. This seems wrong, based on the current doc, but I could be wrong.
I'm also attaching a rebased patch.
Changed 12 years ago by
Attachment:  trac_11311rebased.patch added 

comment:12 Changed 12 years ago by
With the current patch, we have a little code duplication, so we could make the following change:

sage/misc/latex.py
diff git a/sage/misc/latex.py b/sage/misc/latex.py
a b def view(objects, title='SAGE', debug=Fa 1851 1851 else: 1852 1852 latex_options = {} 1853 1853 s = _latex_file_(objects, title=title, sep=sep, tiny=tiny, debug=debug, **latex_options) 1854 if engine is None: 1855 engine = _Latex_prefs._option["engine"] 1856 if pdflatex or (viewer == "pdf" and engine == "latex"): 1857 engine = "pdflatex" 1854 1858 # notebook 1855 1859 if EMBEDDED_MODE and viewer is None: 1856 1860 jsMath_okay = True … … def view(objects, title='SAGE', debug=Fa 1862 1866 if jsMath_okay: 1863 1867 print JSMath().eval(objects, mode=mode) # put comma at end of line? 1864 1868 else: 1865 if engine is None:1866 engine = _Latex_prefs._option["engine"]1867 if pdflatex or (viewer == "pdf" and engine == "latex"):1868 engine = "pdflatex"1869 1869 base_dir = os.path.abspath("") 1870 1870 png_file = graphics_filename(ext='png') 1871 1871 png_link = "cell://" + png_file … … def view(objects, title='SAGE', debug=Fa 1874 1874 print '<html><img src="%s"></html>'%png_link # put comma at end of line? 1875 1875 return 1876 1876 # command line or notebook with viewer 1877 if engine is None:1878 engine = _Latex_prefs._option["engine"]1879 if pdflatex or (viewer == "pdf" and engine == "latex"):1880 engine = "pdflatex"1881 1877 tmp = tmp_dir('sage_viewer') 1882 1878 tex_file = os.path.join(tmp, "sage.tex") 1883 1879 open(tex_file,'w').write(s)
Otherwise, I think it looks good.
As far as opening in a pdf viewer rather than in a TeX program, hasn't that been the behavior for a while? (Sage basically opens up the file, blah.pdf or blah.dvi, using some sensible default depending on the OS  see the file misc/viewer.py. On OS X, for example, this just calls "open blah.pdf" or "open blah.dvi", depending on which file is produced.)
comment:13 Changed 11 years ago by
Description:  modified (diff) 

I'm attaching a referee patch which removes the code duplication referred to above.
KarlDieter: the program "sageopen", as defined in sage/misc/viewer.py, should be used to open the output file. Before the patch here, it was called with output_file
set to "sage.dvi" or something like that. After the patch, it is called with output_file
set to "sage.pdf". So any difference in what program is being used is because this is producing a pdf file, not a dvi file.
I'd like to change this from "needs info" to "positive review". Is that okay?
comment:15 Changed 11 years ago by
Status:  needs_review → positive_review 

comment:16 Changed 11 years ago by
Milestone:  sage4.7.2 → sage4.7.3 

comment:17 Changed 11 years ago by
Authors:  Franco Saliola → Franco Saliola, John Palmieri 

Reviewers:  KarlDieter Crisman → KarlDieter Crisman, Franco Saliola 
comment:18 Changed 11 years ago by
Reviewers:  KarlDieter Crisman, Franco Saliola → KarlDieter Crisman, Franco Saliola, John Palmieri 

comment:20 Changed 11 years ago by
Merged in:  → sage4.8.alpha1 

Milestone:  → sage4.8 
Resolution:  → fixed 
Status:  positive_review → closed 
comment:21 Changed 11 years ago by
Merged in:  sage4.8.alpha1 

Resolution:  fixed 
Status:  closed → new 
I get doctest failures on a system which does not have latex
installed:
sage t long force_lib devel/sage/sage/misc/latex.py ********************************************************************** File "/Users/jdemeyer/sage4.8.alpha1/devel/sagemain/sage/misc/latex.py", line 1877: sage: view(4, engine="garbage") Expected: Traceback (most recent call last): ... ValueError: Unsupported LaTeX engine. Got: Error: XeLaTeX does not seem to be installed. Download it from ctan.org and try again. Latex error ********************************************************************** File "/Users/jdemeyer/sage4.8.alpha1/devel/sagemain/sage/misc/latex.py", line 1882: sage: view(4, engine="garbage", viewer="pdf") Expected: Traceback (most recent call last): ... ValueError: Unsupported LaTeX engine. Got: Error: XeLaTeX does not seem to be installed. Download it from ctan.org and try again. Latex error **********************************************************************
Fixing this would mean adding "# optional  latex" to the doctest.
But it also makes me wonder: why check the existence of XeLaTeX when engine="garbage"
? This doesn't make much sense to me.
comment:22 followup: 23 Changed 11 years ago by
This seems to have been introduced (or expanded, at least) in #8486.
if engine and not have_pdflatex() and not have_xelatex(): print "Error: %s does not seem to be installed. Download it from" % _Latex_prefs._option["engine_name"]
so engine
is certainly True
since it's nonempty, and we definitely don't have any of those things. And it first checks whether one has LaTeX, before it checks the engines.
Now we have
sage: sage.misc.latex._Latex_prefs._option {'engine': 'latex', 'matrix_delimiters': ['(', ')'], 'jsmath_avoid': [], 'engine_name': 'LaTeX', 'blackboard_bold': False, 'macros': '', 'vector_delimiters': ['(', ')'], 'preamble': ''}
in a regular install. The question is whether there is anywhere engine
gets reset...
Aha! Line 1404 (of the current 4.7.2) shows that this is set to xelatex
and then not reset. Apparently the doctest framework doesn't clear this, I'm not sure exactly where this all lives.
Solution should be:
 Fix the reset of the engine
 Leave the error message and
# random
it. Or even include both error messages so that the documentation shows exactly what we expect with and without a working LaTeX installation.
What do you think?
comment:23 Changed 11 years ago by
Replying to kcrisman:
What do you think?
I think we should try to find a solution which leaves all doctests as they are (including the new ones on this ticket): change the code, not the tests.
Maybe the code should be more like:
if engine is None: engine = "latex" if (engine=="latex" and not have_latex()) or (engine=="pdflatex" and not have_pdflatex()) or (engine=="xelatex" and not have_xelatex): raise RuntimeError,...
comment:24 Changed 11 years ago by
Status:  new → needs_review 

Here's a new referee patch. This just moves the code which checks the value of 'engine' earlier, so if 'engine' is set to a bad value, then that error gets caught first. For me, on an OS X box (with a pretty good TeX installation), on sage.math (with a mediocre TeX installation (no xetex or xelatex), and on the skynet machine eno (no TeX at all), the following passes all tests, repeatedly:
sage t randorder devel/sage/sage/misc/latex.py
(The patch also adds a line to one example to reset the latex preamble, so that tests pass when the order of the tests is randomized.)
comment:25 Changed 11 years ago by
Description:  modified (diff) 

Here's an alternate version, which just consolidates some code (while checking whether engine == 'pdflatex'
, check whether pdflatex is installed, for example).
comment:27 Changed 11 years ago by
ping
What is the status of this patch ?
The bot has not understood which patch to apply. Let us try again.
Apply trac_11311rebased.patch and trac_11311ref.v2.patch
comment:29 Changed 11 years ago by
Status:  needs_review → positive_review 

Thanks for the review patch. Positive review.
comment:30 Changed 11 years ago by
I just noticed that randomizing the doctests fails sometimes, with errors of the following form:
sage t randorder devel/sage/sage/misc/latex.py ... File "/Users/saliola/Applications/sage5.0/devel/sage/sage/misc/latex.py", line 221: sage: builtin_constant_function(NotImplemented) Expected: '\\mbox{\\rm NotImplemented}' Got: '{\\rm NotImplemented}' ...
Since this isn't cause by this patch (in fact, this patch improves things), I created a new ticket #12986 for this.
comment:31 Changed 11 years ago by
Status:  positive_review → needs_work 

The patch only applies with fuzz 2 and therefore should be rebased to sage5.1.beta0.
comment:32 Changed 11 years ago by
Description:  modified (diff) 

Status:  needs_work → positive_review 
Here's a rebased patch (combining the old two patches). (Despite its name and my comment when I attached it, it was actually rebased to Sage 5.1.beta0.)
comment:33 Changed 11 years ago by
Merged in:  → sage5.1.beta1 

Resolution:  → fixed 
Status:  positive_review → closed 
comment:34 Changed 11 years ago by
Merged in:  sage5.1.beta1 

Resolution:  fixed 
Status:  closed → new 
Nothing against this patch, just didn't mean to merge it now (probably next beta).
comment:35 Changed 11 years ago by
Status:  new → needs_review 

comment:36 Changed 11 years ago by
Status:  needs_review → positive_review 

comment:37 Changed 11 years ago by
Merged in:  → sage5.1.beta4 

Resolution:  → fixed 
Status:  positive_review → closed 
You should modify the header of your patch : replace
trac_11311: fix view to not overwrite engine="pdflatex"
by
#11311: fix view to not overwrite engine="pdflatex"
Then the buildbot will be more happy.