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: | sage-5.1 |
| Component: | graphics | Keywords: | latex, pdflatex, days30 |
| Cc: | John Palmieri | Merged in: | sage-5.1.beta4 |
| Authors: | Franco Saliola, John Palmieri | Reviewers: | Karl-Dieter Crisman, Franco Saliola, John Palmieri |
| Report Upstream: | N/A | Work issues: | |
| Branch: | Commit: | ||
| Dependencies: | Stopgaps: |
Status badges
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_11311-5.0.patch.
Attachments (6)
Change History (43)
Changed 12 years ago by
| Attachment: | trac_11311_engine_pdflatex_view-fs.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_view-fs.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: | → Karl-Dieter 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_11311-rebased.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.
Karl-Dieter: the program "sage-open", 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: | sage-4.7.2 → sage-4.7.3 |
|---|
comment:17 Changed 11 years ago by
| Authors: | Franco Saliola → Franco Saliola, John Palmieri |
|---|---|
| Reviewers: | Karl-Dieter Crisman → Karl-Dieter Crisman, Franco Saliola |
comment:18 Changed 11 years ago by
| Reviewers: | Karl-Dieter Crisman, Franco Saliola → Karl-Dieter Crisman, Franco Saliola, John Palmieri |
|---|
comment:20 Changed 11 years ago by
| Merged in: | → sage-4.8.alpha1 |
|---|---|
| Milestone: | → sage-4.8 |
| Resolution: | → fixed |
| Status: | positive_review → closed |
comment:21 Changed 11 years ago by
| Merged in: | sage-4.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/sage-4.8.alpha1/devel/sage-main/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/sage-4.8.alpha1/devel/sage-main/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 follow-up: 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
# randomit. 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_11311-rebased.patch and trac_11311-ref.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/sage-5.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 sage-5.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: | → sage-5.1.beta1 |
|---|---|
| Resolution: | → fixed |
| Status: | positive_review → closed |
comment:34 Changed 11 years ago by
| Merged in: | sage-5.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: | → sage-5.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.