Opened 5 years ago

Closed 5 years ago

#17284 closed defect (fixed)

Fix command-line plotting keywords

Reported by: jdemeyer Owned by:
Priority: blocker Milestone: sage-6.4
Component: graphics Keywords:
Cc: vbraun, kcrisman, strogdon Merged in:
Authors: Volker Braun Reviewers: Steven Trogdon, Karl-Dieter Crisman, Jeroen Demeyer, Martin von Gagern
Report Upstream: N/A Work issues:
Branch: c84805c (Commits) Commit: c84805cafa9c5d1310145c9e31cd59e31fb7668f
Dependencies: Stopgaps:

Description (last modified by 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):

  • src/sage/repl/display/formatter.py

    diff --git a/src/sage/repl/display/formatter.py b/src/sage/repl/display/formatter.py
    index 670d208..a352a54 100644
    a b class SageConsoleTextFormatter(SagePlainTextFormatter): 
    377377        """
    378378        from sage.structure.sage_object import SageObject
    379379        if isinstance(obj, SageObject) and hasattr(obj, '_graphics_'):
    380             gfx = obj._graphics_()
    381             if gfx:
    382                 gfx.launch_viewer()
    383                 return ''
     380            obj.show()
     381            return ''
    384382        return super(SageConsoleTextFormatter, self).__call__(obj)

Change History (33)

comment:1 Changed 5 years ago by jdemeyer

  • Description modified (diff)

comment:2 Changed 5 years ago by kcrisman

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

comment:3 Changed 5 years ago by strogdon

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.

Last edited 5 years ago by strogdon (previous) (diff)

comment:4 Changed 5 years ago by vbraun

IPython notebook doesn't support 3d plots yet.

comment:5 Changed 5 years ago by strogdon

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.

comment:6 Changed 5 years ago by vbraun

True, but neither is the desired outcome

comment:7 Changed 5 years ago by vbraun

  • Branch set to u/vbraun/fix_command_line_plotting_keywords

comment:8 Changed 5 years ago by vbraun

  • Authors set to Volker Braun
  • Commit set to c84805cafa9c5d1310145c9e31cd59e31fb7668f
  • Status changed from new to needs_review

New commits:

2915e62Also use the viewer=... option to decide on 3d graphics output
c6c090eMake sure that _extra_kwds is always a dictionary
c84805cWarn about invalid viewer options

comment:9 follow-up: Changed 5 years ago by kcrisman

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.

comment:10 follow-up: Changed 5 years ago by vbraun

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.

comment:11 Changed 5 years ago by jdemeyer

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

comment:12 Changed 5 years ago by jdemeyer

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.

comment:13 in reply to: ↑ 9 Changed 5 years ago by jdemeyer

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.

comment:14 follow-up: Changed 5 years ago by 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.

comment:15 in reply to: ↑ 14 Changed 5 years ago by jdemeyer

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)

comment:16 follow-up: Changed 5 years ago by 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. 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.

comment:17 follow-up: Changed 5 years ago by 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

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.

Last edited 5 years ago by strogdon (previous) (diff)

comment:18 in reply to: ↑ 16 Changed 5 years ago by jdemeyer

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.

comment:19 Changed 5 years ago by vbraun

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

comment:20 in reply to: ↑ 17 Changed 5 years ago by vbraun

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.

comment:21 Changed 5 years ago by kcrisman

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_()? <and that discussion>

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.

comment:22 Changed 5 years ago by vbraun

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.

comment:23 follow-ups: Changed 5 years ago by gagern

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.

comment:24 in reply to: ↑ 23 Changed 5 years ago by kcrisman

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.

comment:25 in reply to: ↑ 23 ; follow-up: Changed 5 years ago by vbraun

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.

comment:26 in reply to: ↑ 25 ; follow-up: Changed 5 years ago by kcrisman

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.

comment:27 in reply to: ↑ 26 Changed 5 years ago by jdemeyer

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.

comment:28 Changed 5 years ago by vbraun

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.

comment:29 in reply to: ↑ 10 Changed 5 years ago by kcrisman

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.

comment:30 Changed 5 years ago by kcrisman

  • Reviewers set to Steven Trogdon, Karl-Dieter Crisman, Jeroen Demeyer, Martin von Gagern

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.

comment:31 Changed 5 years ago by vbraun

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

comment:32 Changed 5 years ago by fbissey

  • Status changed from needs_review to positive_review

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.

comment:33 Changed 5 years ago by vbraun

  • Branch changed from u/vbraun/fix_command_line_plotting_keywords to c84805cafa9c5d1310145c9e31cd59e31fb7668f
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.