Import JSAnimation into the animation module. (Fixes #4703) #4821

dopplershift · 2015-07-29T19:03:44Z

This pulls http://github.com/jakevdp/JSAnimation into the code. Most of
this is in the HTMLWriter class. This also adds the jshtml option for
the animation.html setting.

jenshnielsen · 2015-07-30T19:39:59Z

lib/matplotlib/animation.py

+                                             **mode_dict))
+
+
+def anim_to_jshtml(anim, fps=None, embed_frames=True, default_mode=None):


Is there a specific reason why this is not a method on the animation class?

Because the JSAnimation package had it this way

Because it calls a specific backend, and none of the other backends get a method

Because.....

I don't have a strong preference here. Anyone have an opinion?

I don't have any strong preference. I was just comparing it to the HTLM5 method. I realise that is different now because it's not really a new backend. Either is fine with me.

jenshnielsen · 2015-07-30T19:50:49Z

Nice. Seems to work great. It should get a whats new entry and I would like to make sure that we probably credit the work of @jakevdp I guess there is no easy way of retaining the original commit history? At the least it should be mentioned clearly in the whats new entry.

It probably also makes sense to get a reference added to the original repository mentioning that this is now part of Matplotlib when we get close to a release with this in.

In the long term I think we should replace the present icons with Fort Awesome http://fortawesome.github.io/Font-Awesome/ which we already use in the nbagg backend and which IPython/Jupyter uses internally but that can be done in a separate PR and we don't need to hold this up on that.

The tests failures are pep8 and what looks like random failures on Python 2.7 and 3.4

dopplershift · 2015-07-30T20:24:02Z

I absolutely want to credit @jakevdp. I briefly tried to turn the JSAnimation history for html_writer.py into a set of patches, but trying to apply them as mods to our animation.py was just too much for me to handle. I'd be happy to change the author on these commits if it seems like the right thing to do...

jenshnielsen · 2015-07-30T20:32:30Z

I think you are right that applying the original commits as patches is near impossible. I think the most important is that the comments, documentation and whats new correctly reflects the origin of the module

dopplershift · 2015-07-30T21:41:36Z

I went ahead and used the Font Awesome icons, to avoid having the original icons in the history. It also removed some code and decreased the size of the generated HTML.

jenshnielsen · 2015-07-30T22:04:58Z

Awesome :) I think this is ready to go but I will leave it a little bit to see if anyone else have comments.

tacaswell · 2015-07-31T13:09:53Z

I suspect that we are going to have be blacklist the animation module from the pep8 tests (unless you want to linewrap html), figure out how make it skip block quotes, or have a data file with the html blob.

WeatherGod · 2015-07-31T13:11:30Z

can't you do # noqc on the statement in question?

On Fri, Jul 31, 2015 at 9:09 AM, Thomas A Caswell notifications@github.com
wrote:

I suspect that we are going to have be blacklist the animation module from
the pep8 tests (unless you want to linewrap html), figure out how make it
skip block quotes, or have a data file with the html blob.

—
Reply to this email directly or view it on GitHub
#4821 (comment)
.

dopplershift · 2015-07-31T14:52:01Z

I was planning to go ahead and wrap it when I get a chance today.

I don't want anyone to later pep8 my file :)

jakevdp · 2015-07-31T22:25:26Z

The reason I never submitted this to mpl is that it's a pretty hackish solution to the goal: it embeds raw png binary data as base64 strings directly in the resulting HTML string. This works OK for animations consisting of a few frames, but as soon as you try to do something marginally bigger the file size blows up. I've had reports from people who crashed their IPython session by trying to make a simple animation.

With these limitations in mind, I deliberately chose not to put JSAnimation on PyPI, and not to actually cut a code release, because I didn't feel like dealing with the inevitable flow of issue reports from users who don't understand the quirks of the code. That said, I'm fine with this being in matplotlib, but do keep in mind what it would mean to maintain and support this kind of partial & limited solution.

dopplershift · 2015-08-01T00:43:45Z

@jakevdp I think the HTML5 video solution I just added fills the need for larger animations. I do think the JSAnimation support is useful due to the better controls you can get--provided you don't have too many frames. (These kinds of issues are why it defaults to off.)

efiring · 2015-08-01T03:48:21Z

I can't look at the PR now; but if it is accepted, will the limitations be abundantly clear to the user? Jake's message seems to me like a pretty strong warning that we might regret including JSAnimation in mpl. We have difficulty keeping up with bugs and user queries as it is. I don't think "useful" is sufficient justification at this point.

dopplershift · 2015-08-01T03:55:33Z

I'm willing to document, but the problem is no different than creating too many image objects in a Python session with the standalone animation support. At some point users who do too much will run into problems--I can bring the notebook to a standstill with a print statement and a stupid loop.

I have an animation with 360 images open using JSAnimation right now, with no problems whatsoever--so we're not exactly talking about something really delicate. My opinion is that we will continue to have far more problems reliably running ffmpeg and mencoder across all platforms than displaying PNG images using javascript in the browser.

jakevdp · 2015-08-01T12:32:19Z

the problem is no different than creating too many image objects in a Python session with the standalone animation support.

The difference is that the HTML output can be persistent.

Say someone is using IPython and accidentally creates too many image objects in a conventional animation, bogging down their kernel. They can restart the kernel and try again. No harm done.

Say, however, that someone is using the notebook and accidentally embeds gigabytes of pngs into the HTML output, crashing their browser. The only way to recover the notebook is to somehow strip that output, but you can't do that via any of the IPython tools that try to first open the notebook. Unless the user is comfortable editing JSON by hand to remove the offending lines (and has a program that can handle editing a very large file), the notebook is basically lost.

I'm not saying that JSAnimation shouldn't be added to mpl; I'm just saying it's a hacked solution and we should be well aware of that before committing it.

dopplershift · 2015-08-01T14:41:31Z

How about adding a limit to size of embedded (base64 encoded) content? This could be another rcparam, working for both this and the html5 video stuff. Default to....250 MB?

jakevdp · 2015-08-01T16:01:28Z

How about adding a limit to size of embedded (base64 encoded) content?

I like that! Rather than an rcparam, though, it could be an argument to the converter. Then if the user tries something too big. Maybe set it purposely low, like ~10MB, and have a very informative early error message (maybe based on the size of the first frame * number of frames) then the user can explicitly allow a huge animation if that's what they want.

dopplershift · 2015-08-01T17:16:33Z

My only problem with no rcparam is how to set it for the IPython display hook.

WeatherGod · 2015-08-27T02:08:23Z

Anything holding up this PR? This PR happens to fix a seg-fault for me as mentioned in #4967.

tacaswell · 2015-08-27T02:13:23Z

@jakevdp 's concern (which I am not sure if @dopplershift addressed well enough) and understanding why this fixes the segfault.

dopplershift · 2015-08-27T16:24:57Z

There's the question of whether it should be an rcparam or an explicit argument to the converter (I implemented the former), but otherwise the concerns are addressed IMO.

jakevdp · 2015-09-01T16:09:59Z

lib/matplotlib/animation.py

-                                options=' '.join(options))
+            return VIDEO_TAG.format(video=self._base64_video,
+                                    size=self._video_size,
+                                    options=' '.join(options))


This should not be indented, right?

ping. I also think this was not intentional?

No, it was indented so that we only return the tag if the base64-encoded video data was created (i.e. not too large). The mistake was not to have an alternative return if things go wrong. Will fix.

Not clear if this has been addressed?

There's an else right below this comment now. :)

jakevdp · 2015-09-01T16:16:42Z

Looks pretty good – I noted a few things. Have you run tests on this branch? That indented code should have broken some things.

Also, there is a package that has forked JSAnimation and had to add some script to fix filenames which had spaces in them. I can't say I fully understand the source of the error, but this PR has their fix to the issue.

tacaswell · 2015-09-01T17:09:16Z

@dopplershift I broke this by cherry picking one of your commits into #4995

tacaswell · 2017-08-27T18:09:46Z

attn @choldgraf @NelleV This is probably the right representation to try to grab to put into sphinx-gallery output.

tacaswell · 2017-08-31T01:58:58Z

Can the js and template code be moved to a private module (just to keep animation.py a bit closer to under control)?

dopplershift · 2017-08-31T03:04:19Z

Done.

anntzer · 2017-08-31T03:12:16Z

lib/matplotlib/animation.py

+    def new_id(cls):
+        #return '%16x' % cls.rng.getrandbits(64)
+        return ''.join(cls.rng.choice(string.ascii_uppercase)
+                       for x in range(16))


Just use https://docs.python.org/3.6/library/uuid.html (whichever version you like) and drop rng? (at least I don't think .rng should be exposed)

Good idea, done.

anntzer · 2017-08-31T03:14:40Z

lib/matplotlib/animation.py

+        # Can't open a second time while opened on windows. So we avoid
+        # deleting when closed, and delete manually later.
+        with tempfile.NamedTemporaryFile(suffix='.html', delete=False) as f:
+            anim.save(f.name, writer=HTMLWriter(fps=fps,


Can't we just write to a BytesIO? (when using a subprocess to encode this would be tricky due to the need to use pipes but here it should be straightforward, no?)

I'd love to, and it makes sense, but I think that's a more invasive a change to the original code (which I didn't write) than I'm comfortable with. Animation itself currently assumes a string filename since it relies heavily on command line tools for writing.

Alternatively, in order to make things work on Windows more easily (I have to deal with that all the time :-() you can create a TemporaryDirectory and use whatever filename you want in it -- then you still get proper cleanup at contextmanager exit. Although that probably means depending on https://github.com/pjdelport/backports.tempfile on AntiquePython (or https://github.com/minrk/backports.temporarydirectory but that doesn't appear to be on PyPI)

TBH I still prefer the BytesIO option, but heh.

It just requires adding an in-memory option alongside the existing file option, and using that for the jshtml converter. Just a little late in the game to be adding that, especially given that I didn't write the HTMLWriter.

anntzer · 2017-08-31T03:15:15Z

lib/matplotlib/animation.py

+                if vid_len >= embed_limit:
+                    import warnings
+                    warnings.warn('Animation movie is {0} bytes, exceeding '
+                            'the limit of {1}. If you\'re sure you want a '


Use double quotes around and you won't need to escape single quotes.
No need to number the "{}"'s (here and throughout the PR).

anntzer · 2017-08-31T03:17:17Z

lib/matplotlib/animation.py


    def _repr_html_(self):
        '''IPython display hook for rendering.'''
        fmt = rcParams['animation.html']
        if fmt == 'html5':
            return self.to_html5_video()
+        elif fmt == 'jshtml':
+            return anim_to_jshtml(self)


any reason why anim_to_jshtml is a freestanding function and not a method like to_html5_video? (in which case it should be to_jshtml)
(FTR I don't have anything against freestanding functions, it's more an API consistency thing)

I think because this started as an external code, so the function made sense. I've changed to make things more consistent.

anntzer · 2017-08-31T03:18:31Z

lib/matplotlib/_animation.py

@@ -0,0 +1,210 @@
+# Javascript template for HTMLWriter


Perhaps _animation_data.py? (like _color_data.py)

dopplershift · 2017-08-31T04:18:21Z

I think I resolved @anntzer 's comments.

This pulls http://github.com/jakevdp/JSAnimation into the code. Most of this is in the HTMLWriter class. This also adds the `jshtml` option for the animation.html setting.

dopplershift · 2017-08-31T17:45:18Z

Grrr. Fixed pep8 line length issues.

dopplershift · 2017-08-31T23:52:13Z

Not sure if I want to kick AppVeyor again or not, there seems to be some sporadic problem while it runs test_artists. Last time it failed during 2nd job, this time it was during the 3rd job.

anntzer · 2017-08-31T23:56:16Z

lib/matplotlib/animation.py

+            imgdata64 = encodebytes(f.read()).decode('ascii')
+            self._total_bytes += len(imgdata64)
+            if self._total_bytes >= self._bytes_limit:
+                import warnings


move the import to top?

Heh, it's already there. Removed a few instances of the import lower down.

anntzer · 2017-08-31T23:56:54Z

lib/matplotlib/animation.py

+                         reflect_checked='')
+        mode_dict[self.default_mode + '_checked'] = 'checked'
+
+        interval = int(1000. / self.fps)


1000, no dot (in a py3 world "/" definitely means float div :-))

Tends to happen in 2 year old PRs... 😉

Actually should probably just use // since it's casting to int anyway.

anntzer · 2017-08-31T23:58:17Z

lib/matplotlib/animation.py

+            import warnings
+            warnings.warn("unrecognized default_mode: using 'loop'")
+
+        self._saved_frames = list()


anntzer · 2017-08-31T23:58:44Z

lib/matplotlib/animation.py

+        """Generate HTML representation of the animation"""
+        if fps is None and hasattr(self, '_interval'):
+            # Convert interval in ms to frames per second
+            fps = 1000. / self._interval


anntzer · 2017-09-01T00:01:04Z

lib/matplotlib/animation.py

+            self.fig.savefig(f, format=self.frame_format,
+                             dpi=self.dpi, **savefig_kwargs)
+            f.seek(0)
+            imgdata64 = encodebytes(f.read()).decode('ascii')


f.getvalue(), then you don't need to seek first
shouldn't you just use bytes (and open the final file in binary mode) throughout?

Too much template processing with format to use bytes. Then I'd just have to be using encode everywhere, which doesn't feel like a win.

Used getvalue.

The argument regarding format sounds reasonable.

This makes both the HTMLWriter and the HTML5 video support check the amount of data being saved into HTML against the rc parameter "animation.embed_limit".

dopplershift · 2017-09-01T00:19:41Z

Alright, one more time.

tacaswell added the status: needs review label Jul 29, 2015

jenshnielsen reviewed Jul 30, 2015
View reviewed changes

dopplershift force-pushed the add-jsanimation branch from 56fba88 to 74e6175 Compare July 30, 2015 21:40

tacaswell added this to the proposed next point release milestone Jul 31, 2015

dopplershift force-pushed the add-jsanimation branch 2 times, most recently from 371e7bb to 0657c02 Compare August 1, 2015 03:14

dopplershift mentioned this pull request Aug 20, 2015

Animations are (mostly) broken #4967

Closed

jakevdp reviewed Sep 1, 2015
View reviewed changes

dopplershift force-pushed the add-jsanimation branch from 9f3b4d1 to 957cd3c Compare August 31, 2017 03:04

anntzer reviewed Aug 31, 2017

View reviewed changes

dopplershift force-pushed the add-jsanimation branch from 957cd3c to abf8cc5 Compare August 31, 2017 04:14

Import JSAnimation into the animation module.

34b6ea3

This pulls http://github.com/jakevdp/JSAnimation into the code. Most of this is in the HTMLWriter class. This also adds the `jshtml` option for the animation.html setting.

dopplershift force-pushed the add-jsanimation branch from abf8cc5 to 268d7a0 Compare August 31, 2017 17:44

anntzer reviewed Aug 31, 2017

View reviewed changes

anntzer reviewed Sep 1, 2017

View reviewed changes

dopplershift and others added 4 commits August 31, 2017 18:17

Limit size of HTML embedded animations.

11f0464

This makes both the HTMLWriter and the HTML5 video support check the amount of data being saved into HTML against the rc parameter "animation.embed_limit".

Add What's New file for JSAnimation.

c0c8fe3

ENH: Add HTMLWriter to animation smoke tests

ace6700

Some cleanups to HTMLWriter.

ffb2ccd

dopplershift force-pushed the add-jsanimation branch from 268d7a0 to ffb2ccd Compare September 1, 2017 00:18

tacaswell merged commit e5b9f7d into matplotlib:master Sep 1, 2017

QuLogic removed the status: needs review label Sep 1, 2017

dopplershift deleted the add-jsanimation branch September 1, 2017 03:12

mgeier mentioned this pull request Jul 29, 2018

Un-align animations created with to_jshtml()? #11795

Closed

		**mode_dict))


		def anim_to_jshtml(anim, fps=None, embed_frames=True, default_mode=None):

Import JSAnimation into the animation module. (Fixes #4703) #4821

Import JSAnimation into the animation module. (Fixes #4703) #4821

Conversation

dopplershift commented Jul 29, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jenshnielsen commented Jul 30, 2015

dopplershift commented Jul 30, 2015

jenshnielsen commented Jul 30, 2015

dopplershift commented Jul 30, 2015

jenshnielsen commented Jul 30, 2015

tacaswell commented Jul 31, 2015

WeatherGod commented Jul 31, 2015

dopplershift commented Jul 31, 2015

jakevdp commented Jul 31, 2015

dopplershift commented Aug 1, 2015

efiring commented Aug 1, 2015 via email

dopplershift commented Aug 1, 2015

jakevdp commented Aug 1, 2015

dopplershift commented Aug 1, 2015

jakevdp commented Aug 1, 2015

dopplershift commented Aug 1, 2015

WeatherGod commented Aug 27, 2015

tacaswell commented Aug 27, 2015

dopplershift commented Aug 27, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jakevdp commented Sep 1, 2015

tacaswell commented Sep 1, 2015

tacaswell commented Aug 27, 2017

tacaswell commented Aug 31, 2017

dopplershift commented Aug 31, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anntzer Aug 31, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anntzer Aug 31, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dopplershift commented Aug 31, 2017

dopplershift commented Aug 31, 2017

dopplershift commented Aug 31, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dopplershift commented Sep 1, 2017

anntzer Aug 31, 2017 •

edited

Loading

anntzer Aug 31, 2017 •

edited

Loading