Opened 4 years ago

Closed 4 years ago

#18176 closed defect (fixed)

Show animation

Reported by: vbraun Owned by:
Priority: blocker Milestone: sage-6.6
Component: graphics Keywords:
Cc: novoselt, gagern Merged in:
Authors: Martin von Gagern Reviewers: Volker Braun
Report Upstream: N/A Work issues:
Branch: 9a48ae0 (Commits) Commit: 9a48ae00a4cf86aa5c3cbf2c092e641b9f0c6e59
Dependencies: Stopgaps:

Description (last modified by vbraun)

Bandaid until we can implement a proper fix at #17783

Change History (36)

comment:1 Changed 4 years ago by vbraun

  • Authors set to Volker Braun
  • Component changed from PLEASE CHANGE to graphics
  • Description modified (diff)
  • Type changed from PLEASE CHANGE to defect

comment:2 Changed 4 years ago by vbraun

  • Branch set to u/vbraun/show_animation

comment:3 Changed 4 years ago by vbraun

  • Commit set to 39263a4fc8c59879373a47d55a9040a5d6702de9
  • Status changed from new to needs_review

New commits:

39263a4Remove optional arguments to show since its all broken

comment:4 Changed 4 years ago by kcrisman

  • Cc novoselt added

Blocker?

Anyway, were the optional arguments previously broken, had no one tested with the optional tag, or was this a recent change? Andrey's email suggests recent...

comment:5 Changed 4 years ago by vbraun

The code is only tested with --optional=imagemagick, so in other words it was not tested.

comment:6 Changed 4 years ago by vbraun

And I don't have a particular preference for whether to make it a blocker or not. If somebody cares about animations then they can always review this ticket before sage-6.6 is out (i.e. very soon)

comment:7 Changed 4 years ago by kcrisman

  • Cc gagern added

comment:8 Changed 4 years ago by kcrisman

  • Priority changed from major to blocker

Given that Andrey may not have had a chance to respond about this yet, I'm moving it to blocker until either I get a chance to review it (though all seems straightforward) or he can say it's not that high. a works, just not a.show. Does it seems like Martin's changes or the display changes are more likely to have caused this?

comment:9 Changed 4 years ago by novoselt

  • Priority changed from blocker to critical

I don't know when it got broken - I was thinking I should check how animations work in SageMathCell with my recent changes and discovered they don't work at all in Sage.

The real problem is that they were not even tested.

And my concern with changes on this ticket is that it will break a.show(delay=10) in user code.

But if it is up to me to decide, this is not a blocker.

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

I tried to preserve the delay parameter, but then I don't even know what its supposed to mean. Sometimes its the numerator of some fraction (with a separate delay_denominator. Sometimes its centiseconds, because thats super-convenient right. Never mind why you would ever pass a fraction as separate numerator/denominator in Sage. IMHO it should be the reciprocal anyways, fps = frames per second. That is much more common when talking about animation speeds.

comment:11 Changed 4 years ago by gagern

What exactly do you mean by “its all broken”? It would be kind of nice if the problem report were to report the problem, before suggesting a fix… In any case, I'm on 6.6.beta1 here, using the Sage notebook, and with imagemagick 6.9.0.3 installed on my system. And animations with delay and iterations do work for me. To be more specific,

sage: a = animate([sin(x + float(k)) for k in srange(0,2*pi,0.3)], xmin=0, xmax=2*pi, figsize=[2,1])
sage: a.show(delay=10, iterations=2) # shows at higher frame rate then stops
sage: a.show(delay=500)              # shows at lower frame rate

So what exactly is broken?

comment:12 in reply to: ↑ 10 ; follow-up: Changed 4 years ago by gagern

Replying to vbraun:

I tried to preserve the delay parameter, but then I don't even know what its supposed to mean.

The doc says correctly:

  • delay between frames (measured in hundredths of a second, default value 20)
  • (default: 20) delay in hundredths of a second between frames

This is used throughout the public Animation class. You might argue that it's a suboptimal choice, but it's what the GIF spec uses, what most animated GIF editors use, and what Sage has been using so far.

Sometimes its the numerator of some fraction (with a separate delay_denominator. Sometimes its centiseconds, because thats super-convenient right. Never mind why you would ever pass a fraction as separate numerator/denominator in Sage.

That's in the APngAssembler I wrote, and it's there that way because I consider the APngAssembler a low-level interface, and the low level spec of APNG uses that representation for rational numbers. I saw no reason to complicate things further by introducing my own translation layer, and I didn't consider delay_denominator harmful enough to fix it at 100 instead of leaving the caller an option to override this.

IMHO it should be the reciprocal anyways, fps = frames per second. That is much more common when talking about animation speeds.

For movies, yes. For animated GIFs, in my experience no. I wouldn't mind having fps as an additional argument, so users can choose between specifying that or delay. But I'd not kill off the delay argument without proper deprecation. And don't you dare call this spacebar heating!

comment:13 Changed 4 years ago by gagern

I notice that delay and iterations only have limited support from ffmpeg. The docs say as much. What does sage.misc.sage_ostools.have_program("convert") report for you?

comment:14 in reply to: ↑ 12 Changed 4 years ago by vbraun

Replying to gagern:

it's what the GIF spec uses, what most animated GIF editors use

By the same line of argument I suggest we replace seconds everywhere with jiffies, since this is what the kernel developers use. If you go to the movies, whats the frame delay in centiseconds? How many frames per second? ;-)

What is broken:

  • testing (0% coverage unless you specifically set --optional)
  • save doesn't respect delay, iterations (show goes through save)

comment:15 Changed 4 years ago by vbraun

And I do have imagemagick installed, the tests are just not run by default (See also #13540)

comment:16 Changed 4 years ago by gagern

When was the last time you had a Sage-generated movie play at a cinema in your parts? On a more serious note, I believe that delay makes a lot of sense if one speaks of a number associated to individual frames, but less so if it's the same for all of them. Which is a reason why delay makes more sense in a GIF environment than an MPEG one.

show does go through save only since your commit 7747261. It was calling gif before that. In dc0067d for #7298 I had written a code path which routed show through save without breaking such stuff. Unfortunately, #7298 won't merge since it conflicts with your changes, and I haven't yet found the time to rewrite it appropriately.

Test coverage is tricky. Since we don't bundle imagemagick, we can't rely on convert so we can't create GIF. The same holds for ffmpeg and the various video formats. And even if we had these tools enabled, how do we automatically verify that the file looks as it should? I guess we could disassemble the file to some textual representation of all its settings, but that would require considerable amounts of code. Or we could set up something like Selenium based tests. Although it's hard to get timing right with those. But I'm against punting a useful feature just because we can't auto-test it as well as we'd like.

comment:17 follow-up: Changed 4 years ago by vbraun

Test coverage should be easy since PIL(low) can be used to write animated GIF, we just don't use it.

In any case, not every optional argument for every potential image file format needs to be exposed via show()

comment:18 follow-up: Changed 4 years ago by vbraun

Animations will be totally broken in Sage-6.6 since apparently that is preferable to not supporting some optional keyword argument. Propose to close this as wontfix until we can rewrite the animations so that no keyword is left behind.

comment:19 in reply to: ↑ 18 ; follow-up: Changed 4 years ago by kcrisman

Animations will be totally broken in Sage-6.6 since apparently that is preferable to not supporting some optional keyword argument. Propose to close this as wontfix until we can rewrite the animations so that no keyword is left behind.

I don't have any objection (well, not enough to break animations) to this temp fix, I just don't have time to properly do a review. As I say,

all seems straightforward

so please let's make animations still work. Or at least, animation show method, I guess without show it still works, correct?

comment:20 in reply to: ↑ 19 Changed 4 years ago by kcrisman

Animations will be totally broken in Sage-6.6 since apparently that is preferable to not supporting some optional keyword argument. Propose to close this as wontfix until we can rewrite the animations so that no keyword is left behind.

I don't have any objection (well, not enough to break animations) to this temp fix, I just don't have time to properly do a review. As I say,

Actually, I will just make time.

comment:21 Changed 4 years ago by kcrisman

  • Priority changed from critical to blocker

comment:22 Changed 4 years ago by kcrisman

Okay, this is weird. I get it working in the command line now, but

**********************************************************************
File "src/sage/plot/animate.py", line 34, in sage.plot.animate
Failed example:
    a         # optional -- ImageMagick
Exception raised:
      File "<doctest sage.plot.animate[0]>", line 1, in <module>
        a         # optional -- ImageMagick
    NameError: name 'a' is not defined
**********************************************************************

when running tests. ???

Version 0, edited 4 years ago by kcrisman (next)

comment:23 Changed 4 years ago by vbraun

I know, the tests with enabling optional stuff are generally broken.

comment:24 Changed 4 years ago by kcrisman

Oh, duh! I have to do all or sage as well - http://www.sagemath.org/doc/developer/doctesting.html#run-optional-tests. Sorry, my bad.

Last edited 4 years ago by kcrisman (previous) (diff)

comment:25 Changed 4 years ago by kcrisman

Give me another hour - I have to run an time-constrained errand right now but will return to this shortly.

comment:26 Changed 4 years ago by gagern

  • Branch changed from u/vbraun/show_animation to u/gagern/t/17783/animationSaveKwds

comment:27 Changed 4 years ago by gagern

  • Authors changed from Volker Braun to Martin von Gagern
  • Commit changed from 39263a4fc8c59879373a47d55a9040a5d6702de9 to 14dd2818961df94bafdc2e2d1f2b640d3a68d5ae

Here is a branch which preserves the functionality of keyword arguments to show by passing them on through save to gif. It even has doctests to ensure that the delay and iteration count actually end up in the created file. And I fixed one other doctest failure introduced by 7747261. Now things work for me in the Sage Notebook, and sage -bt --long --optional=all src/sage/plot/animate.py is happy as well. Please review.


New commits:

e56fbeaTrac #18176: Pass keywords from show through save to gif resp. ffmpeg
14dd281Don't use file name argument to show method

comment:28 Changed 4 years ago by vbraun

  • Reviewers set to Volker Braun
  • Status changed from needs_review to positive_review

comment:29 Changed 4 years ago by vbraun

  • Branch changed from u/gagern/t/17783/animationSaveKwds to 14dd2818961df94bafdc2e2d1f2b640d3a68d5ae
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:30 Changed 4 years ago by kcrisman

  • Commit 14dd2818961df94bafdc2e2d1f2b640d3a68d5ae deleted

Wow, you pick up your kid, and the world keeps rotating ;-) Nice!!!

comment:31 Changed 4 years ago by vbraun

  • Resolution fixed deleted
  • Status changed from closed to new
[plotting ] /mnt/disk/home/buildslave-sage/slave/sage_git/build/local/lib/python2.7/site-packages/sage/plot/animate.py:docstring of sage.plot.animate.Animation.save:49: WARNING: Block quote ends without a blank line; unexpected unindent.
Error building the documentation.
Traceback (most recent call last):
  File "/mnt/disk/home/buildslave-sage/slave/sage_git/build/src/doc/common/builder.py", line 1626, in <module>
    getattr(get_builder(name), type)()
  File "/mnt/disk/home/buildslave-sage/slave/sage_git/build/src/doc/common/builder.py", line 292, in _wrapper
    getattr(get_builder(document), 'inventory')(*args, **kwds)
  File "/mnt/disk/home/buildslave-sage/slave/sage_git/build/src/doc/common/builder.py", line 503, in _wrapper
    x.get(99999)
  File "/mnt/disk/home/buildslave-sage/slave/sage_git/build/local/lib/python/multiprocessing/pool.py", line 558, in get
    raise self._value
OSError: [plotting ] /mnt/disk/home/buildslave-sage/slave/sage_git/build/local/lib/python2.7/site-packages/sage/plot/animate.py:docstring of sage.plot.animate.Animation.save:49: WARNING: Block quote ends without a blank line; unexpected unindent.

comment:32 in reply to: ↑ 17 Changed 4 years ago by gagern

Replying to vbraun:

Test coverage should be easy since PIL(low) can be used to write animated GIF, we just don't use it.

It can? A quick search of the web returned several posts where people talked about animated GIF read support, or the absence of write support, or asked about write support and got no answer. If you are certain about this, and perhaps even know an address with some documentation, could you open a ticket for this?

In any case, not every optional argument for every potential image file format needs to be exposed via show()

We had that discussion in comment:50:ticket:7298 and following, and I still disagree. Let's continue this aspect in this thread on the sage-devel list, so we can get more opinons on this.

comment:33 Changed 4 years ago by gagern

  • Branch changed from 14dd2818961df94bafdc2e2d1f2b640d3a68d5ae to u/gagern/14dd2818961df94bafdc2e2d1f2b640d3a68d5ae

comment:34 Changed 4 years ago by gagern

  • Branch changed from u/gagern/14dd2818961df94bafdc2e2d1f2b640d3a68d5ae to u/gagern/t/17783/animationSaveKwds

comment:35 Changed 4 years ago by gagern

  • Commit set to 9a48ae00a4cf86aa5c3cbf2c092e641b9f0c6e59
  • Status changed from new to needs_review

Just forgot to use r""" for the docstring once I started using escape sequences in string literals within that docstring.


New commits:

e56fbeaTrac #18176: Pass keywords from show through save to gif resp. ffmpeg
14dd281Don't use file name argument to show method
9a48ae0Use raw mode for docstring

comment:36 Changed 4 years ago by vbraun

  • Branch changed from u/gagern/t/17783/animationSaveKwds to 9a48ae00a4cf86aa5c3cbf2c092e641b9f0c6e59
  • Resolution set to fixed
  • Status changed from needs_review to closed
Note: See TracTickets for help on using tickets.