Opened 7 years ago

Closed 7 years ago

Last modified 7 years ago

#16608 closed defect (fixed)

Immediate fix for animations in notebook

Reported by: niles Owned by:
Priority: blocker Milestone: sage-6.3
Component: notebook Keywords: notebook, animate
Cc: kcrisman, jhpalmieri, gagern Merged in:
Authors: Niles Johnson Reviewers: Martin von Gagern
Report Upstream: N/A Work issues:
Branch: 16cbd2d (Commits, GitHub, GitLab) Commit:
Dependencies: Stopgaps:

Status badges

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

This is a regression introduced by #12827. See discussion at #16533.

A good fix for this is in progress as part of a larger effort to unify and expand animation functionality. This ticket simply restores the functionality in the meantime.

Change History (11)

comment:1 Changed 7 years ago by niles

  • Status changed from new to needs_review

See discussion at #16533:24 and below.

I've run all long doctests successfully.

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

comment:2 follow-up: Changed 7 years ago by kcrisman

gagern says there:

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.

The change is minimal indeed. I can't check myself whether this works but certainly the code should make it work.

I've run all long doctests successfully.

Did you run them with optional tests enabled? ;-)

comment:3 in reply to: ↑ 2 Changed 7 years ago by niles

Replying to kcrisman:

I've run all long doctests successfully.

Did you run them with optional tests enabled? ;-)

ha ha; no, I forgot. Now

sage -tp --long --optional=sage,ImageMagick,convert src/sage/plot

passes all tests. Of course, none of them test embedded mode anyway!

comment:4 follow-up: Changed 7 years ago by gagern

Note that users who want to use FFmpeg to generate their GIF will have to pass an explicit name to the ffmpeg method, since in your code show does not support a use_ffmpeg argument and save does not make a suitable choice of file name. Not sure this is worth fixing, though.

comment:5 in reply to: ↑ 4 ; follow-up: Changed 7 years ago by niles

Replying to gagern:

Note that users who want to use FFmpeg to generate their GIF will have to pass an explicit name to the ffmpeg method, since in your code show does not support a use_ffmpeg argument and save does not make a suitable choice of file name. Not sure this is worth fixing, though.

For this ticket, let's focus only on fixing regressions from previous functionality. Did those things work before #12827? With the current branch, all of the following work in the notebook:

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

[shows the gif]
c.save(show_path=True)

[saves and returns path '/Users/njohnson/.sage/temp/n15158/65775/tmp_zk_bPS.gif']
c.save(show_path=True,use_ffmpeg=True)

[saves and returns path '/Users/njohnson/.sage/temp/n15158/65775/tmp_x8eYhm.gif']
c.save('test.mpg',show_path=True,use_ffmpeg=True)

[saves and returns path '/private/var/folders/vm/9s3hrqs50jgfjhdl20yl9yjnfj74f3/T/tmpJY9m2M/test.mpg'

The animations exist at the indicated locations in the indicated formats, and ffmpeg is used when requested.

Examples from the reference manual involving implicit_plot3d and Tachyon also work as expected.

comment:6 in reply to: ↑ 5 Changed 7 years ago by gagern

Replying to niles:

For this ticket, let's focus only on fixing regressions from previous functionality. Did those things work before #12827?

The way I read the diff, it should have worked, since both these methods called graphics_filename. The documentation also indicates as much, e.g. the docstring for gif() writes:

        If ``savefile`` is not specified: in notebook mode, display the
        animation; otherwise, save it to a default file name.

        EXAMPLES::

            sage: a = animate([sin(x + float(k)) for k in srange(0,2*pi,0.7)],
            ....:                xmin=0, xmax=2*pi, figsize=[2,1])
            sage: dir = tmp_dir()
            sage: a.gif()              # not tested

This “display the animation” part of the promise is still broken in your fix, and the a.gif() call doesn't have a lot of effect since it saves the file in some temporary directory without telling anyone its location.

I must confess that my own changes from #16533 won't fix this either. Only with #16573 did I change the documentation in a way which is not backwards-compatible but at least restores a match between documentation and behavior.

An alternative to your fix would be getting #15515 ready and the replacing all occurrences of tmp_filename with graphics_filename. That should restore old behavior in all relevant cases.

Of course, we could also merge your fix and then build all subsequent modifications on that. After all, your fix is better than no fix at all, and it looks like I might have to rebase most of my animation-related changes anyway.

comment:7 follow-ups: Changed 7 years ago by kcrisman

Of course, we could also merge your fix and then build all subsequent modifications on that. After all, your fix is better than no fix at all, and it looks like I might have to rebase most of my animation-related changes anyway.

Martin, if the fix solves your problem and you agree with tests then let's get this in. I can only test simple animations, I don't have ffmpeg installed, but it does appear to work correctly and as I said the code seems right. I'm not a big fan of making things rebase, as I find it very tedious, but this is high enough priority that we should just fix it.

This “display the animation” part of the promise is still broken in your fix

True. Maybe just as a hack, one could always add show_path=True for EMBEDDED_MODE... that might end up annoying. Maybe just when if not savefile and EMBEDDED_MODE one could make savefile=graphics_filename(ext='.gif') ? Would that work? Too hackish?

Also, Niles, you can actually test things with embedded mode... maybe we should add that.

But we should do it soon!

comment:8 in reply to: ↑ 7 Changed 7 years ago by niles

Replying to kcrisman:

Also, Niles, you can actually test things with embedded mode... maybe we should add that.

But we should do it soon!

I agree, but unfortunately I am traveling this week and probably also unavailable for the next two weeks. I think this is borderline but just over the threshold for a positive review, but I'll leave that up to you. I think the options are:

  • Positive review as is
  • Get someone else to add changes, make more robust
  • Downgrade from blocker

I know this isn't a great situation, but I just can't do more at the moment.

comment:9 in reply to: ↑ 7 ; follow-up: Changed 7 years ago by gagern

  • Reviewers set to Martin von Gagern
  • Status changed from needs_review to positive_review

Replying to kcrisman:

Martin, if the fix solves your problem and you agree with tests then let's get this in.

Fine. Let's get this over with, and I'll have a look at rebases and proper fixes after that.

This “display the animation” part of the promise is still broken in your fix

True. Maybe just as a hack, one could always add show_path=True for EMBEDDED_MODE... that might end up annoying. Maybe just when if not savefile and EMBEDDED_MODE one could make savefile=graphics_filename(ext='.gif') ? Would that work? Too hackish?

Too hackish. Properly fixing this isn't that much harder. It would mean doing the distinction between embedded mode and non-embedded mode for every occurrence of tmp_filename. Which is a lot of code duplication unless you move that distinction into graphics_filename, which is what #15515 is about. Maybe I can review that one soon, too, but don't hold your breath on me since I've an important appointment coming up.

Also, Niles, you can actually test things with embedded mode... maybe we should add that.

Do you mean test suite? Can we run tests inside the notebook server somehow? And see where things end up, what html gets generated and so on? Or am I completely missing the point of what you meant?

In any case, changing tests at this point is probably a bad idea. Should be a separate issue.

comment:10 Changed 7 years ago by vbraun

  • Branch changed from u/niles/ticket/16533-stopgap to 16cbd2d59b4b5bbb330668255345fc2d53f3afee
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:11 in reply to: ↑ 9 Changed 7 years ago by gagern

  • Commit 16cbd2d59b4b5bbb330668255345fc2d53f3afee deleted

I just implemented a proper fix including gif and ffmpeg in #16645. Does one of you want to review that? Niles perhaps?

Note: See TracTickets for help on using tickets.