Opened 9 years ago
Closed 9 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, GitHub, GitLab) | 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 9 years ago by
Branch: | → u/gagern/ticket/16533 |
---|---|
Created: | Jun 25, 2014, 7:55:28 AM → Jun 25, 2014, 7:55:28 AM |
Modified: | Jun 25, 2014, 7:55:28 AM → Jun 25, 2014, 7:55:28 AM |
comment:2 Changed 9 years ago by
Commit: | → 1f7de6eb7fa8bffdf54e1a73d6c8a0ae4051ffd1 |
---|
comment:3 Changed 9 years ago by
Commit: | 1f7de6eb7fa8bffdf54e1a73d6c8a0ae4051ffd1 → 0c21bd8d4257cb8c96560880a84e827552324378 |
---|
comment:4 Changed 9 years ago by
Status: | new → needs_review |
---|
comment:5 Changed 9 years ago by
Commit: | 0c21bd8d4257cb8c96560880a84e827552324378 → b8a3c7c3affeffea7d3054e69f18065204c5d930 |
---|
comment:6 follow-up: 8 Changed 9 years ago by
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 9 years ago by
Commit: | b8a3c7c3affeffea7d3054e69f18065204c5d930 → 51007fbf59595127ed4b784155e91d78ebb52b2a |
---|
Branch pushed to git repo; I updated commit sha1. New commits:
51007fb | Inline animations in tables.
|
comment:8 follow-up: 9 Changed 9 years ago by
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 follow-ups: 10 11 Changed 9 years ago by
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
callsave
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:
- Common APNG and <video>:
- APNG support:
- Documentation improvements:
- <video> tag support:
- 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.
comment:10 Changed 9 years ago by
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 follow-up: 12 Changed 9 years ago by
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 Changed 9 years ago by
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 9 years ago by
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 9 years ago by
Branch: | u/gagern/ticket/16533 → u/gagern/ticket/16533b |
---|---|
Commit: | 51007fbf59595127ed4b784155e91d78ebb52b2a → 0325ee23e8e339d7ad2a3f36f6c0b03da7a0cafb |
comment:15 Changed 9 years ago by
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: 17 18 Changed 9 years ago by
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 follow-up: 19 Changed 9 years ago by
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 Changed 9 years ago by
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 Changed 9 years ago by
Replying to niles:
Moreover, when I do
c.save()
I get nothing, but when I doc.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 9 years ago by
Priority: | major → 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 9 years ago by
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: 24 Changed 9 years ago by
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 9 years ago by
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.
comment:24 Changed 9 years ago by
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: 26 Changed 9 years ago by
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 Changed 9 years ago by
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 9 years ago by
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: 30 Changed 9 years ago by
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 9 years ago by
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 Changed 9 years ago by
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 9 years ago by
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 9 years ago by
Priority: | blocker → major |
---|---|
Status: | needs_review → needs_work |
comment:33 Changed 9 years ago by
Commit: | 0325ee23e8e339d7ad2a3f36f6c0b03da7a0cafb → f7d57dc09566c02ab010a9c01eaae563100dc396 |
---|
comment:34 Changed 9 years ago by
Just a pointer about the temporary filenames: see also #15515 (which I admit, needs work).
comment:35 Changed 9 years ago by
Owner: | set 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:
- Fix
show
: #16608 - Fix
gif
andffmpeg
: #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.
comment:36 follow-up: 37 Changed 9 years ago by
Milestone: | sage-6.3 → 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 follow-up: 38 Changed 9 years ago by
Status: | needs_work → 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 Changed 9 years ago by
Status: | needs_review → 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 9 years ago by
Resolution: | → duplicate |
---|---|
Status: | positive_review → closed |
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:
Create HTML to embed generated GIF in notebook.
Return path to generated file.
Avoid leaking animation files.
Base Animation.show on Animation.save, and pass more arguments.
Add APNG support.