Opened 5 years ago

Closed 5 years ago

#16533 closed defect (duplicate)

Notebook won't show animations

Reported by: gagern Owned by: gagern
Priority: major Milestone: sage-duplicate/invalid/wontfix
Component: graphics Keywords:
Cc: kcrisman, jhpalmieri Merged in:
Authors: Martin von Gagern Reviewers:
Report Upstream: N/A Work issues:
Branch: u/gagern/ticket/16533b (Commits) Commit: f7d57dc09566c02ab010a9c01eaae563100dc396
Dependencies: Stopgaps:

Description

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

Change History (39)

comment:1 Changed 5 years ago by gagern

  • Branch set to u/gagern/ticket/16533
  • Created changed from 06/25/14 07:55:28 to 06/25/14 07:55:28
  • Modified changed from 06/25/14 07:55:28 to 06/25/14 07:55:28

comment:2 Changed 5 years ago by gagern

  • Commit set to 1f7de6eb7fa8bffdf54e1a73d6c8a0ae4051ffd1

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:

9374cffCreate HTML to embed generated GIF in notebook.
84886ceReturn path to generated file.
8199b02Avoid leaking animation files.
1d29d17Base Animation.show on Animation.save, and pass more arguments.
1f7de6eAdd APNG support.

comment:3 Changed 5 years ago by git

  • Commit changed from 1f7de6eb7fa8bffdf54e1a73d6c8a0ae4051ffd1 to 0c21bd8d4257cb8c96560880a84e827552324378

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

f3a5b90Avoid multiple acTL hunks in APNG file.
4c9a491Turn generator into list when rendering frames.
0c21bd8Improved documentation.

comment:4 Changed 5 years ago by gagern

  • Status changed from new to needs_review

comment:5 Changed 5 years ago by git

  • Commit changed from 0c21bd8d4257cb8c96560880a84e827552324378 to b8a3c7c3affeffea7d3054e69f18065204c5d930

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

3edfb7fSupport HTML5 video tag.
5725460Fix doc test.
b8a3c7cMore control over video tag attributes.

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

comment:7 Changed 5 years ago by git

  • Commit changed from b8a3c7c3affeffea7d3054e69f18065204c5d930 to 51007fbf59595127ed4b784155e91d78ebb52b2a

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

51007fbInline animations in tables.

comment:8 in reply to: ↑ 6 ; follow-up: Changed 5 years ago by 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.

comment:9 in reply to: ↑ 8 ; follow-ups: Changed 5 years ago by 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:

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.

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

comment:10 in reply to: ↑ 9 Changed 5 years ago by 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.

comment:11 in reply to: ↑ 9 ; follow-up: Changed 5 years ago by 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.

comment:12 in reply to: ↑ 11 Changed 5 years ago by 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.

comment:13 Changed 5 years ago by kcrisman

  • Cc kcrisman added

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.

comment:14 Changed 5 years ago by gagern

  • Branch changed from u/gagern/ticket/16533 to u/gagern/ticket/16533b
  • Commit changed from 51007fbf59595127ed4b784155e91d78ebb52b2a to 0325ee23e8e339d7ad2a3f36f6c0b03da7a0cafb

OK, I'm splitting things up:

I hope Trac will pick up my new branch now.


New commits:

ac83bdbProper hyperlink formatting in animate documentation.
031b97eBase Animation.show on Animation.save, and pass more arguments.
0325ee2Create HTML to embed generated GIF in notebook.

comment:15 Changed 5 years ago by kcrisman

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

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

comment:17 in reply to: ↑ 16 ; follow-up: Changed 5 years ago by 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 . . .

comment:18 in reply to: ↑ 16 Changed 5 years ago by 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.

comment:19 in reply to: ↑ 17 Changed 5 years ago by 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.

comment:20 Changed 5 years ago by kcrisman

  • Priority changed from major to blocker

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.

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

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

comment:23 Changed 5 years ago by niles

  • Cc jhpalmieri added

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.

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

comment:24 in reply to: ↑ 22 Changed 5 years ago by 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?

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

comment:26 in reply to: ↑ 25 Changed 5 years ago by 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.

comment:27 Changed 5 years ago by 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?

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

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

comment:30 in reply to: ↑ 28 Changed 5 years ago by 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.

comment:31 Changed 5 years ago by 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 :-)

comment:32 Changed 5 years ago by niles

  • Priority changed from blocker to major
  • Status changed from needs_review to needs_work

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

comment:33 Changed 5 years ago by git

  • Commit changed from 0325ee23e8e339d7ad2a3f36f6c0b03da7a0cafb to f7d57dc09566c02ab010a9c01eaae563100dc396

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

c5039b4Restore positional parameters.
f7d57dcInline another pair of links.

comment:34 Changed 5 years ago by jdemeyer

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

comment:35 Changed 5 years ago by gagern

  • Owner changed from (none) to 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:

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.

comment:36 follow-up: Changed 5 years ago by gagern

  • Milestone changed from sage-6.3 to sage-duplicate/invalid/wontfix

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.

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

  • Status changed from needs_work to needs_review

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

comment:38 in reply to: ↑ 37 Changed 5 years ago by gagern

  • Status changed from needs_review to positive_review

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.

comment:39 Changed 5 years ago by vbraun

  • Resolution set to duplicate
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.