Opened 5 years ago
Closed 5 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 )
Bandaid until we can implement a proper fix at #17783
Change History (36)
comment:1 Changed 5 years ago by
- Component changed from PLEASE CHANGE to graphics
- Description modified (diff)
- Type changed from PLEASE CHANGE to defect
comment:2 Changed 5 years ago by
- Branch set to u/vbraun/show_animation
comment:3 Changed 5 years ago by
- Commit set to 39263a4fc8c59879373a47d55a9040a5d6702de9
- Status changed from new to needs_review
comment:4 Changed 5 years ago by
- 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 5 years ago by
The code is only tested with --optional=imagemagick
, so in other words it was not tested.
comment:6 Changed 5 years ago by
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 5 years ago by
- Cc gagern added
comment:8 Changed 5 years ago by
- 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 5 years ago by
- 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: ↓ 12 Changed 5 years ago by
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 5 years ago by
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: ↓ 14 Changed 5 years ago by
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 5 years ago by
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 5 years ago by
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 5 years ago by
And I do have imagemagick installed, the tests are just not run by default (See also #13540)
comment:16 Changed 5 years ago by
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: ↓ 32 Changed 5 years ago by
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: ↓ 19 Changed 5 years ago by
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: ↓ 20 Changed 5 years ago by
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 5 years ago by
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 5 years ago by
- Priority changed from critical to blocker
comment:22 Changed 5 years ago by
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. In fact, I get this error while doctesting whether or not I apply this! Command
$ ./sage -tp 4 --optional=imagemagick src/sage/plot/
comment:23 Changed 5 years ago by
I know, the tests with enabling optional stuff are generally broken.
comment:24 Changed 5 years ago by
Oh, duh! I have to do all
as well. Sorry, my bad.
comment:25 Changed 5 years ago by
Give me another hour - I have to run an time-constrained errand right now but will return to this shortly.
comment:26 Changed 5 years ago by
- Branch changed from u/vbraun/show_animation to u/gagern/t/17783/animationSaveKwds
comment:27 Changed 5 years ago by
- 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:
e56fbea | Trac #18176: Pass keywords from show through save to gif resp. ffmpeg
|
14dd281 | Don't use file name argument to show method
|
comment:28 Changed 5 years ago by
- Reviewers set to Volker Braun
- Status changed from needs_review to positive_review
comment:29 Changed 5 years ago by
- Branch changed from u/gagern/t/17783/animationSaveKwds to 14dd2818961df94bafdc2e2d1f2b640d3a68d5ae
- Resolution set to fixed
- Status changed from positive_review to closed
comment:30 Changed 5 years ago by
- Commit 14dd2818961df94bafdc2e2d1f2b640d3a68d5ae deleted
Wow, you pick up your kid, and the world keeps rotating ;-) Nice!!!
comment:31 Changed 5 years ago by
- 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 5 years ago by
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 5 years ago by
- Branch changed from 14dd2818961df94bafdc2e2d1f2b640d3a68d5ae to u/gagern/14dd2818961df94bafdc2e2d1f2b640d3a68d5ae
comment:34 Changed 5 years ago by
- Branch changed from u/gagern/14dd2818961df94bafdc2e2d1f2b640d3a68d5ae to u/gagern/t/17783/animationSaveKwds
comment:35 Changed 5 years ago by
- Commit set to 9a48ae00a4cf86aa5c3cbf2c092e641b9f0c6e59
- Status changed from new to needs_review
comment:36 Changed 5 years ago by
- Branch changed from u/gagern/t/17783/animationSaveKwds to 9a48ae00a4cf86aa5c3cbf2c092e641b9f0c6e59
- Resolution set to fixed
- Status changed from needs_review to closed
New commits:
Remove optional arguments to show since its all broken