Notebook won't show animations

[gagern]

The following example, taken from the documentation of the animate module, produces no visible output in the notebook:

sage: sines = [plot(c*sin(x), (-2*pi,2*pi), color=Color(c,0,0), ymin=-1,ymax=1) for c in sxrange(0,1,.2)]
sage: a = animate(sines)
sage: a.show()

CC: @kcrisman @jhpalmieri

Component: graphics

Author: Martin von Gagern

Branch/Commit: u/gagern/ticket/16533b @ f7d57dc

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

[gagern]

Branch: u/gagern/ticket/16533

[gagern]

Commit: 1f7de6e

[gagern]

comment:2

OK, I did a bit more than just enabling the embedding. I did restructure the show and save commands, and added support for APNG. I hope that I might be able to add other formats in the future, in the spirit of ticket:7298.

---

New commits:

9374cff	Create HTML to embed generated GIF in notebook.
84886ce	Return path to generated file.
8199b02	Avoid leaking animation files.
1d29d17	Base Animation.show on Animation.save, and pass more arguments.
1f7de6e	Add APNG support.

[sagetrac-git]

Changed commit from 1f7de6e to 0c21bd8

[sagetrac-git]

Branch pushed to git repo; I updated commit sha1. New commits:

f3a5b90	Avoid multiple acTL hunks in APNG file.
4c9a491	Turn generator into list when rendering frames.
0c21bd8	Improved documentation.

[sagetrac-git]

Changed commit from 0c21bd8 to b8a3c7c

[sagetrac-git]

Branch pushed to git repo; I updated commit sha1. New commits:

3edfb7f	Support HTML5 video tag.
5725460	Fix doc test.
b8a3c7c	More control over video tag attributes.

[gagern]

comment:6

Originally I had intended to push a separate branch for the video tag aspect of this, but now I figure that reviewing a single branch should be easier than dealing with multiple interdependent branches. So this ticket here will take care of that aspect from ticket:7298 as well.

[sagetrac-git]

Branch pushed to git repo; I updated commit sha1. New commits:

51007fb

Inline animations in tables.

[sagetrac-git]

Changed commit from b8a3c7c to 51007fb

[jdemeyer]

comment:8

Replying to @gagern:

I figure that reviewing a single branch should be easier than dealing with multiple interdependent branches.

Personally, I completely disagree with this. I think fixing the notebook animate bug, the <video> tag and the APNG support should be 3 different tickets.

[gagern]

comment:9

Replying to @jdemeyer:

I think fixing the notebook animate bug, the <video> tag and the APNG support should be 3 different tickets.

I guess I could try to split my modifications up. But there are several parts where I would not know where to put them.

• Several documentation improvements are not directly related to either of these issues. Should I file yet another ticket?
• And this whole modification of having show call save is used for APNG and
• The last commit, about inlining animations in tables, is again distinct, even though it was included in the ticket:7298 patch as well. Separate ticket as well?

The way I see it at the moment, we have 5 distinct modifications, two of which share a commit:

• Notebook:
  • Create HTML to embed generated GIF in notebook
  • Return path to generated file
  • Avoid leaking animation files
• Common APNG and
• APNG support:
  • Add APNG support
  • Avoid multiple acTL hunks in APNG file
  • Turn generator into list when rendering frames
  • Fix doc test
• Documentation improvements:
  • Improved documentation
• Tables:
  • Inline animations in tables

It would take some time to work out whether these branches each make sense on their own. I'd prefer to only invest that effort if I know it is actually needed. Would you be willing to review some of these aspects, Jeroen? If so, that would be one reason for formatting things in the way you prefer.

[jdemeyer]

comment:10

Replying to @gagern:

I'd prefer to only invest that effort if I know it is actually needed.

That is totally understandable.

Would you be willing to review some of these aspects, Jeroen?

Sorry, but probably not. I just looked at this ticket, hoping for a "quick fix" to the issue mentioned in the ticket. But I personally don't care that much.

[jdemeyer]

comment:11

Replying to @gagern:

I guess I could try to split my modifications up. But there are several parts where I would not know where to put them.

My rule is: when in doubt, make a new ticket. Generally, that makes things easiest to review. Imagine there is a reviewer who cares very much about the <video> tag. He could easily review one of your 5 hypothetical tickets, but he might find this ticket here too overwhelming to review.

[gagern]

comment:12

Replying to @jdemeyer:

My rule is: when in doubt, make a new ticket.

I can understand your argument. On the other hand, reviewing a branch officially entails running the complete test suite, including long tests. I'd rather do that once only instead of five times. But I guess I'll have a look at how much trouble splitting this stuff actually is, and act according to that. If I can simply cherry-pick the commits as described above, and they pass doctests, that's probably what I'll do.

[kcrisman]

comment:13

I completely agree that this ticket should first only take care of missing animations.

Also, is this at all related to this sage-support discussion? It should be clear exactly when/where this is necessary.

[gagern]

Changed branch from u/gagern/ticket/16533 to u/gagern/ticket/16533b

[gagern]

comment:14

OK, I'm splitting things up:

• Notebook: here
• Documentation: Animate example looks broken #16570
• APNG: APNG (Animated PNG) for animations #16571

I hope Trac will pick up my new branch now.

---

New commits:

ac83bdb	Proper hyperlink formatting in animate documentation.
031b97e	Base Animation.show on Animation.save, and pass more arguments.
0325ee2	Create HTML to embed generated GIF in notebook.

[gagern]

Changed commit from 51007fb to 0325ee2

[kcrisman]

comment:15

See also #15081, though I'm not sure which (if any) of the followup tickets are directly related to that one.

[kcrisman]

comment:16

Okay, I'm 99% sure that #12827 caused this. Do we really need all this infrastructure for this to be fixed? (I'm not saying we don't, just checking.)

[nilesjohnson]

comment:17

Replying to @kcrisman:

Okay, I'm 99% sure that #12827 caused this. Do we really need all this infrastructure for this to be fixed? (I'm not saying we don't, just checking.)

Ok; I worked on #12827, and I confess I didn't think to check functionality in the notebook. The following is indeed broken, but I don't know why (I haven't read this ticket yet):

c = animate([circle((i,i), 1-1/(i+1), hue=i/10) for i in srange(0,2,0.2)],xmin=0,ymin=0,xmax=2,ymax=2,figsize=[2,2])
c.show()

Moreover, when I do

c.save()

I get nothing, but when I do

c.save('tmp.gif')

the gif is saved and is displayed. I'll investigate further . . .

[gagern]

comment:18

Replying to @kcrisman:

Okay, I'm 99% sure that #12827 caused this.

I agree, namely the two instances where sage.misc.temporary_file.graphics_filename got replaced by tmp_filename.

Do we really need all this infrastructure for this to be fixed? (I'm not saying we don't, just checking.)

Not neccessarily. We could get away with just replacing tmp_filename by graphics_filename if we are in the EMBEDDED_MODE case. This is essentially undoing the graphics_filename removal from #12827. Although I would advise against unconditionally using graphics_filename. I did that at some point, and suddenly my test suite started bleeding files into the current working directory from where I ran the tests. That's why I wrote 8199b02 in my now-superseded branch.

The reason to have all this infrastructure is twofold. On the one hand, it also facilitates later improvements, like #7298 and #16571. On the other hand, it makes code more similar to Graphics.show, which helps maintainability. So I really suggest you compare Animation.show to Graphics.show when reviewing this. There you see common aspects:

• Delegating to save
• Passing kwargs
• Using graphics_filename only in the embedded case

I think this commonness justifies the larger footprint of my modification, but if you want to aim for a fix which is as small as possible, then concentrating on the tmp_filename vs. graphics_filename distinction is sufficient.

[gagern]

comment:19

Replying to @nilesjohnson:

Moreover, when I do c.save() I get nothing, but when I do c.save('tmp.gif')
the gif is saved and is displayed.

The former saves in a temporary directory which the notebook knows nothing about, whereas the latter saves in the current directory where the notebook will find the file and display it to the user. Also note #16573 about this “I get nothing” user experience.

[kcrisman]

comment:20

Hopefully you and Niles can come to some resolution of this that

• fixes the prime problem asap
• leaves in enough infrastructure to build on
• doesn't make doing bigger animations hard again
  I'm making this a blocker since it seems that there is a quick resolution possible and we really don't want this kind of very visible regression any longer than possible.

[kcrisman]

comment:31

Great discussion! But let's keep in mind that probably in this particular case the most important thing is that Sage 6.3 allows animations to work again. Niles' branch doesn't allow me to look at various commits, but I think that just making this work first is most important, even if that means a slight amount of rebasing for the "right fix". Let's put it this way - I don't think a one-liner change really will mean much difference for gagern's code, because he can just put that in another ticket and even use the same exact branch :-)

[nilesjohnson]

comment:32

I've created a ticket for the immediate fix at #16608 (blocker) and downgraded this ticket from blocker -- please review #16608 :)

[sagetrac-git]

Changed commit from 0325ee2 to f7d57dc

[sagetrac-git]

Branch pushed to git repo; I updated commit sha1. New commits:

c5039b4	Restore positional parameters.
f7d57dc	Inline another pair of links.

[jdemeyer]

comment:34

Just a pointer about the temporary filenames: see also #15515 (which I admit, needs work).

[gagern]

comment:35

I guess I should close this ticket here, since various derived tickets will take care of the distinct aspects. Since I can't do so now, I'll check whether I can after accepting it first.

#16608 and #16645 are the most immediate fix to the problem described in the ticket description. Here is a list of related tickets:

• Fix show: Immediate fix for animations in notebook #16608
• Fix gif and ffmpeg: Restore displaying animations in notebook #16645 based on Change graphics_filename() to return a tmp_filename() except from the notebook #15515
• File names in 3d plotting: Graphics3d.show abuses graphics_filename #16640
• <video> and tables: use html5 video tag for animations #7298
• Documentation and doctest example: Animate example looks broken #16570
• APNG support: APNG (Animated PNG) for animations #16571
• A way to deprecate positional parameters: Enforce keyword-only parameters, with deprecation #16607

At the moment, there is no commit in any of these to tweak _sage_argspec_ for Animation.show(). I guess I might eventually write a wrapper for that, something like @delegates_to which would copy the names of parameters from other commands. But I'm not certain that this would indeed be a good idea, so I guess it should be discussed first, perhaps on the mailing list. With so many patches awaiting review at the moment, I don't feel like implementing yet another feature just now.

[gagern]

comment:36

OK, even as the owner of the ticket I can't close it. Too bad. So let's call this a duplicate now, and be done with it. If anyone with sufficient privileges stumbles upon this, feel free to close.

[kcrisman]

comment:37

OK, even as the owner of the ticket I can't close it. Too bad. So let's call this a duplicate now, and be done with it. If anyone with sufficient privileges stumbles upon this, feel free to close.

Correct, but you can put it to positive review. The release manager is the only person supposed to close tickets, just so we have everything tracked properly. But this workflow is the equivalent of that in other projects with "committers".

[gagern]

comment:38

Replying to @kcrisman:

Correct, but you can put it to positive review. The release manager is the only person supposed to close tickets, just so we have everything tracked properly.

Thanks for the clarification. I guess I thought of “review” mostly in terms of “code review” and therefore didn't apply it to situations where there is no code to review.