Fix command-line plotting keywords

[jdemeyer]

This should open a Tachyon-rendered plot (and this worked until very recently):

sage: cube(viewer="tachyon")

The following patch fixes the problem (but probably not in the correct way):

diff --git a/src/sage/repl/display/formatter.py b/src/sage/repl/display/formatter.py
index 670d208..a352a54 100644
--- a/src/sage/repl/display/formatter.py
+++ b/src/sage/repl/display/formatter.py
@@ -377,8 +377,6 @@ class SageConsoleTextFormatter(SagePlainTextFormatter):
         """
         from sage.structure.sage_object import SageObject
         if isinstance(obj, SageObject) and hasattr(obj, '_graphics_'):
-            gfx = obj._graphics_()
-            if gfx: 
-                gfx.launch_viewer()
-                return ''
+            obj.show()
+            return ''
         return super(SageConsoleTextFormatter, self).__call__(obj)

CC: @vbraun @kcrisman @strogdon

Component: graphics

Author: Volker Braun

Branch/Commit: c84805c

Reviewer: Steven Trogdon, Karl-Dieter Crisman, Jeroen Demeyer, Martin von Gagern

Issue created by migration from https://trac.sagemath.org/ticket/17284

[kcrisman]

comment:2

Yeah, I think that was more or less the previous code, huh? But would it break showing graphics correctly in sagenb or ipynb?

[strogdon]

comment:3

This does work here from the command line. The notebook also seems OK. However, the ipynb seems to be malfunctioning. The default in ipynb is Tachyon, i.e. cube() returns a Tachyon rendered image. And cube(viewer='jmol') also returns a Tachyon image. cube().show(viewer='jmol') does return a jmol rendered image in the jmol viewer.

[vbraun]

comment:4

IPython notebook doesn't support 3d plots yet.

[strogdon]

comment:5

I may be mistaken but on a previous trying of the IPython notebook (when I can't remember) something like cube() would spawn the external jmol viewer, which is not the case now.

[vbraun]

comment:6

True, but neither is the desired outcome

[vbraun]

Branch: u/vbraun/fix_command_line_plotting_keywords

[vbraun]

New commits:

2915e62	Also use the viewer=... option to decide on 3d graphics output
c6c090e	Make sure that _extra_kwds is always a dictionary
c84805c	Warn about invalid viewer options

[vbraun]

Commit: c84805c

[vbraun]

Author: Volker Braun

[kcrisman]

comment:9

I can't test it out immediately because I don't have time to compile up to rc1 right now, but this seems okay.

Only one question - there are other viewers (e.g. canvas3d) available in the notebook, so I assume this code will only be reached in the command line? Currently we get, in the command line,

sage: cube().show(viewer='canvas3d')
---------------------------------------------------------------------------
<snip>
UnboundLocalError: local variable 'viewer_app' referenced before assignment

which isn't all that helpful either, I have to admit.

[vbraun]

comment:10

This shouldn't change anything on SageNB.

Other viewers are not supported on the command line, nothing changed there:

sage: cube(viewer='canvas3d')
/home/vbraun/Sage/git-develop/local/lib/python2.7/site-packages/sage/repl/display/formatter.py:380: UserWarning: viewer=canvas3d is not supported
  gfx = obj._graphics_()

Really, the issue is the code quality in the 3d plots. Everything and their dog is lumped in a giant show() method with tons of if-branches, behaving slightly different depending on some global variables. I'll break all that up into separate methods in #17234.

[jdemeyer]

comment:11

Why do we reinvent the show() wheel inside _graphics_()?

[jdemeyer]

comment:12

In other words, what's wrong with:

def _graphics_(self, **kwds):
    self.show(**kwds)

Even if there is something wrong with the above, perhaps the right solution is to fix show() instead of inventing a new function _graphics_ doing mostly the same.

[jdemeyer]

comment:13

Replying to @kcrisman:

Currently we get, in the command line,

sage: cube().show(viewer='canvas3d')
---------------------------------------------------------------------------
<snip>
UnboundLocalError: local variable 'viewer_app' referenced before assignment

This is fixed in #16640 by raising a better error message.

[vbraun]

comment:14

_graphics_ only handles finding the appropriate output. The actual generation of graphics files is dispatched elsewhere. And no global variables allowed.

Everything is wrong with show, its a giant pile of shite. I'm slowly refactoring it into something maintainable if we have 3+ separate output channels.

[jdemeyer]

comment:15

Replying to @vbraun:

_graphics_ only handles finding the appropriate output. The actual generation of graphics files is dispatched elsewhere. And no global variables allowed.

Everything is wrong with show, its a giant pile of shite. I'm slowly refactoring it into something maintainable if we have 3+ separate output channels.

If your solution is to move stuff from show to somewhere else, I agree. Just don't "fix" show by re-inventing its functionality.

(I have seen the following too often: X is broken. Instead of fixing X, somebody invents Y which is similar to X but with other bugs)

[vbraun]

comment:16

The whole premise of using a handful of global variables to steer the output pipeline is IMHO utter crap. It didn't work well when we had a single Notebook, and it sure as hell is a nightmare if we want to deal with 3 different notebooks at the same time. There needs to be some infrastructure to arbitrate between backend capabilities and user preferences. But I sure as hell don't want to reinvent how show() works, I'm getting PTSD whenever I look in there.

[strogdon]

comment:17

Is this the intended behavior? From the notebook

sphere(viewer='java')

/64bitdev/storage/sage-git_develop/sage/local/lib/python2.7/site-package\
s/IPython/core/formatters.py:239: FormatterWarning: Exception in
text/plain formatter: Unknown 3d plot type: java
  FormatterWarning,
None

with no output, which is what I would expect. But from the Sage prompt

/64bitdev/storage/sage-git_develop/sage/local/lib/python2.7/site-packages/sage/repl/display/formatter.py:380: UserWarning: viewer=java is not supported
  gfx = obj._graphics_()

and the jmol viewer is spawned. And if sphere(viewer='java') is typed again there is no warning and the jmol viewer is spawned.

[jdemeyer]

comment:18

Replying to @vbraun:

The whole premise of using a handful of global variables to steer the output pipeline is IMHO utter crap. It didn't work well when we had a single Notebook, and it sure as hell is a nightmare if we want to deal with 3 different notebooks at the same time.

I never said that show() was good, I'm just warning: don't make it worse! IMHO, the introduction of _graphics_ did make things worse but maybe that's just a local minimum.

[vbraun]

comment:19

Had I tacked on the IPython notebook with yet another global variable and extra if branches then it would have caused bugs, too...

[vbraun]

comment:20

Replying to @strogdon:

Is this the intended behavior? From the notebook

sphere(viewer='java')
/64bitdev/storage/sage-git_develop/sage/local/lib/python2.7/site-package\
s/IPython/core/formatters.py:239: FormatterWarning: Exception in
text/plain formatter: Unknown 3d plot type: java
  FormatterWarning,
None

As far as I'm concerned, this is the regrettable state of current affairs but not really on-topic for this ticket. Sphere should validate input when the graphics object is created, not fall flat on its face later on when it is about to be drawn.

[kcrisman]

comment:21

Two comments that hopefully will keep us on target:

Is this the intended behavior? From the notebook sphere(viewer='java') ...

No, but I think this was already the case earlier, although the error messages probably will have changed. We can't necessarily validate EVERY input, as I've been told when I've attempted to on many other tickets... incidentally, there is the java3d viewer or something like it, but again that only works (actually, doesn't, there is a ticket for that too) in the notebook.

Why do we reinvent the show() wheel inside graphics()?

This is a noble goal and I totally agree that the 3d graphics show code is miserable due to accretion of layers over the years, but I think it probably isn't really worth the effort to fix that right now because it will be pretty hard to test for all possible bugs easily. On the other hand, putting something like that in an early beta someday when there aren't actual bugs to fix seems fine.

It's also worth pointing out that one reason that is so messy is because the actual creation of most plots is viewer-agnostic, which was deemed to be "a good thing" by TBDFL.

[vbraun]

comment:22

Creation of plots should of course be viewer-agnostic. The problem is that the modularity stops there and drops into a monolithic if-branch thicket (that is moreover hard to test). There are more modular problems here, like creating the rendering / viewer files, what to do with the output files, arbitrating capabilities versus user preferences.

[gagern]

comment:23

I guess you already know the following; I'm just posting for the sake of completeness. The commit which caused the originally reported problem, i.e. opening JMOL instead of a PNG viewer, was caused by commit e7e7f9c by Volker. Which is not surprising in hindsight: it changed the _graphics_ implementation from self.show() to something which does self.save(filename) with a JMOL file unless some different mime_types gets passed. Which is exactly the location Volker's current branch here is tweaking.

[kcrisman]

comment:24

I guess you already know the following; I'm just posting for the sake of completeness.

Thanks for confirming this.

If you have already tried this and are satisfied with the code and behavior, feel free to put positive review - I haven't been able to upgrade Sage to the right place yet.

[vbraun]

comment:25

Replying to @gagern:

The commit which caused the originally reported problem, i.e. opening JMOL instead of a PNG viewer, was caused by commit e7e7f9c by Volker.

Which is also precisely why the current show() is such a pile of shit: virtually no meaningful doctests, any time you touch something it is almost guaranteed to cause an error somewhere.

[kcrisman]

comment:26

The commit which caused the originally reported problem, i.e. opening JMOL instead of a PNG viewer, was caused by commit e7e7f9c by Volker.

Which is also precisely why the current show() is such a pile of ****: virtually no meaningful doctests, any time you touch something it is almost guaranteed to cause an error somewhere.

True, though that still might not make dealing with it worth the while as compared to other things one could spend time on. That's why I always visually tested output when I reviewed changes in things related to it, though probably one would want to do so in any case - esp. given that we can't do like mpl and have pre-existing files to test against, at least I don't think so.

[jdemeyer]

comment:27

Replying to @kcrisman:

given that we can't do like mpl and have pre-existing files to test against, at least I don't think so.

We could in theory hash the resulting PNG file and check that in a doctest. However, I don't know how workable that would be in practice.

[vbraun]

comment:28

Apart from testing that the output pipeline ends in the place we thought it ends, we should also compare with the resulting rastered image (though was not what I was talking about previously). We can't do pixel comparisons because that might depend on rounding, installed fonts, and/or freetype version. The classical algorithm for that is to break up your picture in a grid (say, 10x10) and compare the color histogram in each sector with a small tolerance.

[kcrisman]

comment:29

This shouldn't change anything on SageNB.

Yup, looks good there now that I had a chance, and this does indeed fix the previous problem.

[kcrisman]

comment:30

Since several people have commented here, I'm reluctant to be the final one to set to positive review. But FWIW I don't hear any complaints about the code itself, just that Sage and show need help and that some undesirable behavior that isn't documented is or isn't still happening. But the issue at hand seems to be fixed.

[kcrisman]

Reviewer: Steven Trogdon, Karl-Dieter Crisman, Jeroen Demeyer, Martin von Gagern

[vbraun]

comment:31

Can somebody grow a pair and set this to positive review already?

[kiwifb]

comment:32

I think all the people commenting here didn't really have a problem with the code, since you don't want to push the button yourself Volker I'll do it on behalf of all those commentators.

[vbraun]

Changed branch from u/vbraun/fix_command_line_plotting_keywords to c84805c