Use six library to ensure pycaffe.py python3 compliance #3716

Merged
merged 1 commit into from Mar 3, 2016

Conversation

Projects
None yet
4 participants

ttdt commented Feb 24, 2016

By replacing itervalues, next and iteritems with six library functions, pycaffe.py is now python3 compliant.

@seanbell seanbell commented on an outdated diff Feb 24, 2016

python/caffe/pycaffe.py
remainder = num % batch_size
- num_batches = num / batch_size
+ num_batches = int(num / batch_size)
@seanbell

seanbell Feb 24, 2016

Contributor

You can use // to get integer divide for both python2 and python3.

ttdt commented Feb 25, 2016

Use // for integer division (python2 and python3 compliant)

shelhamer added the Python 3 label Feb 25, 2016

ttdt closed this Feb 25, 2016

ttdt reopened this Feb 25, 2016

ttdt commented Feb 25, 2016

CI build fails because of random 404 on package download from repo.continuum.io: https://repo.continuum.io/pkgs/free/linux-64/setuptools-20.1.1-py35_0.tar.bz2
I'll try again (close/reopen PR) in a few hours, hoping the bug will be fixed.

ttdt closed this Feb 25, 2016

ttdt reopened this Feb 25, 2016

@longjon longjon commented on an outdated diff Feb 26, 2016

python/caffe/pycaffe.py
@@ -14,6 +14,9 @@
RMSPropSolver, AdaDeltaSolver, AdamSolver
import caffe.io
+# py3 compliance
+from six import itervalues, next, iteritems
@longjon

longjon Feb 26, 2016

Contributor

I guess I'd rather we make our use of six explicit by just doing import six here... I think that's how we already do it in other files, and six is pretty short, and I think it's easier to read that way.

You can also drop the comment, IMO... we all know what six does, and anyone who is not familiar with it can find out easily.

Contributor

longjon commented Feb 26, 2016

Looks good except as noted. We should really have tests that cover these cases...

Contributor

longjon commented Feb 29, 2016

Great, the diff looks good to me now. If you could clean up the history by squashing into a single commit, this will be ready for merge.

ttdt closed this Feb 29, 2016

ttdt commented Feb 29, 2016

PR #3746 opened with a clean history.

Edit: sorry for this mess, i reopened this one (#3716) and closed the new one (#3746).

Owner

shelhamer commented Feb 29, 2016

@ttdt in the future do not close the PR, but instead update the PR by force pushing to your branch. Making new PRs adds noise to the tracker.

@ttdt ttdt Use 'six' library to ensure python3 compliance.
Use '//' instead of '/' for entire division.
666da79

ttdt reopened this Feb 29, 2016

Contributor

longjon commented Mar 3, 2016

Thanks for the Python 3 update, merging!

(By the way, I think you mean "integer division" or "floor division" in the commit message (but it's not worth fixing, just a note for the future)).

@longjon longjon added a commit that referenced this pull request Mar 3, 2016

@longjon longjon Merge pull request #3716 from ttdt/master
Use six library to ensure pycaffe.py python3 compliance
559758d

@longjon longjon merged commit 559758d into BVLC:master Mar 3, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@fxbit fxbit added a commit to Yodigram/caffe that referenced this pull request Sep 1, 2016

@longjon @fxbit longjon + fxbit Merge pull request #3716 from ttdt/master
Use six library to ensure pycaffe.py python3 compliance
86e8a83
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment