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()

[gagern]

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.

[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.

[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]

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.

[git]

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

​51007fb

Inline animations in tables.

[jdemeyer]

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]

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 <video>, so where should it go? Can I create a branch which contains this change, and then use that branch for both APNG and <video>?
• 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 <video>:
  • ​Base Animation.show on Animation.save, and pass more arguments
• 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
• <video> tag support:
  • ​Support HTML5 video tag
  • ​More control over video tag attributes
• 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]

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]

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]

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]

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]

OK, I'm splitting things up:

• Notebook: here
• <video> and tables: #7298
• Documentation: #16570
• APNG: #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.

[kcrisman]

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

[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.)

[niles]

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]

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]

Replying to niles:

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]

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.

[gagern]

One more thing to consider: apparently before #12827, one could call Animation.gif() and Animation.ffmpeg() to have files displayed in the notebook. Neither this patch here nor the one from #16573 will restore that functionality. Only the code path through Animation.show() will be suitable for notebook. If I were to restore that functionality with all my other modifications in mind, then I'd do so in the _adjust_savefile method I introduce in ​005b83f from #16571.

If you want that functionality restored for the next release as well, then please let me know whether one of you is willing to review patches up to that. Note that although #16571 is about adding APNG support, the commit in question occurs before (and in preparation) to that. So you can review patches up to that if you want to. I could even merge that commit to the branch for this ticket here, and then add the use of graphics_filename to that.

I myself am not sure that I would want gif and ffmpeg to show their results unconditionally (as images instead of e.g. paths). But for the sake of backwards compatibility it might still be reasonable.

[niles]

I'm cc-ing jhpalmieri since he gave positive review to the ticket which introduced this regression -- #12827.

If I understand correctly, the minimal necessary change is to use graphics_filename in embedded mode of animate. I've just pushed a branch which does this, and the code from the ticket description does show an animation in the notebook.

Branch: ​u/niles/ticket/16533-stopgap

I think the other changes discussed here are important, but I don't have time right now to understand them fully or review them. It makes sense that the save/show/internal functions of animation objects and other graphics objects should be comparable. But I would need more time to think carefully about the best organization. I'm not sure that the proposed treatment of keyword arguments is ideal, for example.

I propose that we do the most immediate and quick fix to address the regression, and continue work on the other enhancements here in another ticket with a more appropriate description. Anyone interested in giving a positive review?

The outline in comment 9 might make for a good organizational framework, and the new metaticket could be set to depend on a number of different tickets for single issues.

[niles]

John: This ticket addresses a regression we (I) introduced in #12827: Expand Animation class to accept more graphics types

The changes there broke the show method for animations in the notebook.

I've pushed a branch which fixes this -- I'd appreciate if you could take a look.

[gagern]

Replying to niles:

I've just pushed a branch which does this, and the code from the ticket description does show an animation in the notebook.

The stopgap looks fine to me. However, it doesn't scale well to generating videos or APNG files. So if someone decides that my branch isn't going to make it to the next release, then I'm giving your small change a positive review. But that would mean I'll probably have to rebase the branches for several other tickets, so at the moment I hope someone will review my own more extensive modifications.

I'm not sure that the proposed treatment of keyword arguments is ideal, for example.

Could you elaborate on your concerns there?

[jhpalmieri]

I don't have a strong preference between the two proposed fixes; I probably lean toward Martin's, since it fits in with his other plans. Having said that, I think that it's a bad idea to change the keyword arguments from show(self, delay=20, iterations=0) to show(self, format=None, linkmode=False, **kwargs): if someone is currently calling a.show(10, 2), it won't work after the change. The same issue might arise with the changes to the arguments to the save method.

[gagern]

Replying to jhpalmieri:

I think that it's a bad idea to change the keyword arguments […]

Valid point. I whish we had Python 3 with its keyword-only arguments. I guess for the time being I'll simply accept kwargs only, and pop my arguments from that dict. I had something along these lines, copied from Graphics.show but reverted in the now superseded commit ​3edfb7f.

[gagern]

To be more specific: I think that we probably should avoid a situation where a user passed arguments without keywords and these suddenly change their meaning, leading to obscure errors. I think, however, that it is acceptable to outright reject these calls with a clear indication that positional parameters are unsuitable here. All the examples in the docstrings use keywords, so we never advertised the positional use. In that regard, save needs no changes at all. show would have to pop two items from kwargs. I have the change ready to commit locally, but I'll await feedback whether you agree on that deliberate breach of backwards compatibility.

Sure, we could include the old positional parameters, and pass them on to save. One problem here is that we have to repeat default values all over the place. If we decide to change the default delay to something else, or perhaps to not set a default delay at all for some formats, then there will be several places which need modifications. To deal with that, one would have to set all defaults to None at the outer levels, and replace that with the actual default value just where it is actually needed. Doable. Would you prefer a modification along these lines?

[niles]

I'm not sure if I understand the best way to deal with this kind of situation (keyword defaults passed to other methods). I understand that removing duplication is best practice, but I am frustrated when I can't read in a docstring what possible keywords a function takes.

The plot functions seem to have some very complex system for dealing with keywords, maybe resolving this tension -- is it relevant here?

Also, I'd like to make sure we keep the ​deprecation policy in mind (support old syntax for 1 year after warning is issued).

[niles]

Now that I've actually looked at the plot code and the ​@options decorator, I think it was designed to solve a slightly different problem (set global option defaults) but does give a way to set default keywords once and have them appear in the docstring of multiple functions.

Depending how complicated the option handling needs to be, it may or may not be worthwhile to use this decorator.

[gagern]

Replying to niles:

Also, I'd like to make sure we keep the ​deprecation policy in mind (support old syntax for 1 year after warning is issued).

That is a very useful guideline here on how to proceed, thanks a lot! I will restore the positional parameters for now. And I just filed #16607 for a more generic way of deprecating positional parameters, which – if it gets accepted – could be applied here as well.

I'm not sure if I understand the best way to deal with this kind of situation (keyword defaults passed to other methods). I understand that removing duplication is best practice, but I am frustrated when I can't read in a docstring what possible keywords a function takes.

I thought that even when the function only accepts **kwargs, the docstring should still document the most commonly used keywords. In that case, the function signature would be incomplete, but the docsting as a whole would (thanks to unfortunate amounds of dupliate text) still describe these parameters. I would however welcome some idea on how to avoid this duplication as well.

The plot functions seem to have some very complex system for dealing with keywords, maybe resolving this tension -- is it relevant here?

I'll have to investigate all the magic asociated with _sage_argspec_, but now that you mention it, it should be possible to improve the situation here as well. In particular, the @options decorator would be useful for show in any case, since people might have preferences over how to view their animations, particularly with respect to format (GIF / APNG / WebM / Theora). Having to set these in every call can become annoying. Will investigate that.

[kcrisman]

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 :-)

[niles]

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

[git]

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

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

[jdemeyer]

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

[gagern]

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: #16608
• Fix gif and ffmpeg: #16645 based on #15515
• File names in 3d plotting: #16640
• <video> and tables: #7298
• Documentation and doctest example: #16570
• APNG support: #16571
• A way to deprecate positional parameters: #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]

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]

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]

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.