Skip to content

use base64.encodestring on python2.7 #5570

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
Nov 30, 2015
Merged

use base64.encodestring on python2.7 #5570

merged 1 commit into from
Nov 30, 2015

Conversation

tomoemon
Copy link
Contributor

This PR fixes #5508.
I don't know the standard way to solve version compatibility issues in this project. So I chose explicit way.

I've tested a code below with IPython notebook on Python2.7.5 and Python3.4.2.

get_ipython().enable_matplotlib("inline")

import os
import numpy as np
import matplotlib as mpl
import matplotlib.pyplot as plt
mpl.style.use('ggplot')

mpl.rcParams['animation.html'] = 'html5'
mpl.rcParams['animation.ffmpeg_path'] = os.path.expanduser("~/bin/ffmpeg")

fig = plt.figure()
x = np.arange(500)
frame_list = []

for a in range(10):
    frame = plt.plot(x, a * x ** 2, lw=2)
    frame_list.append(frame)

from matplotlib import animation
animation.ArtistAnimation(fig, frame_list, interval = 500)

@jenshnielsen
Copy link
Member

This break another test in test simplification because it checks for the presence of encodebytes.

I would prefer that the code does not monkeypatch the stdlib modules. I would suggest something like

try:
    from base64 import decodebytes
except:
   from base64 import decodestring as decodebytes

and then change L935 to

vid64 = encodebytes(video.read())

@jenshnielsen
Copy link
Member

The pep8 test fails because you need at least 2 spaces before an inline comment

 E261 at least two spaces before inline comment

@tomoemon tomoemon force-pushed the fix_base64_animation branch from e82971b to c7e98aa Compare November 27, 2015 11:32
@tomoemon
Copy link
Contributor Author

@jenshnielsen Thank you for you comment and sorry for my poor testing. I've fixed my commit, but some tests are failing by other reasons. Do you know why that tests fail?

@jenshnielsen
Copy link
Member

Great. The new test failures are just random flukes not related to your changes. I have restarted these tests

@WeatherGod
Copy link
Member

Hmm, coveralls isn't happy for some reason. Shall we ignore this for now? I think we can improve test coverage of the animation module by using the new NullWriter as a smoke tester later.

@jenshnielsen
Copy link
Member

@WeatherGod Yes ignore that. This code is uncovered. Hence why we did not find it earlier and now it's one line longer so the coverage goes down 😢

@tacaswell tacaswell added this to the Critical bugfix release (1.5.1) milestone Nov 30, 2015
WeatherGod added a commit that referenced this pull request Nov 30, 2015
@WeatherGod
Merge pull request #5570 from tomoemon/fix_base64_animation
bc79db4 
use base64.encodestring on python2.7
@WeatherGod WeatherGod merged commit bc79db4 into matplotlib:master Nov 30, 2015
WeatherGod added a commit that referenced this pull request Nov 30, 2015
@WeatherGod
Merge pull request #5570 from tomoemon/fix_base64_animation
2baedb0 
use base64.encodestring on python2.7
@WeatherGod
Copy link
Member

backported to v1.5.x as 2baedb0

@WeatherGod
Copy link
Member

Hmm, not exactly sure, but I am wondering if 1.5.x is now broken for py2.6? I don't think this PR broke it, but for some reason, one of the usetex unittests is breaking for py2.6...

@tomoemon tomoemon deleted the fix_base64_animation branch December 1, 2015 01:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
v1.5.1
Development

Successfully merging this pull request may close these issues.

Animation.to_html5_video requires python3 base64 module
4 participants
@tomoemon @jenshnielsen @WeatherGod @tacaswell