python 3 support via 2to3 using setuptools.

SciTools / biggus

:no_entry: [DEPRECATED] Virtual large arrays and lazy evaluation.

http://biggus.readthedocs.io/

GNU Lesser General Public License v3.0

54 stars 27 forks source link

python 3 support via 2to3 using setuptools. #65

Closed ocefpaf closed 10 years ago

ocefpaf commented 10 years ago

Hi,

I'm trying to get biggus to work with python3. However, I need some help to move forward.

I'm using the "2to3 approach". This adds setuptools as a dependency. I know that some people prefer six, but I think that six is better once the code base is python3 and one wants to make it backwards compatible.
I modified line 1538 from map(None, shape, keys) to list(itertools.izip_longest(shape, keys)). I hope that does not impact performance or anything else.
The only test that fails in my PR is biggus.tests.test_linear_mosaic.TestLinearMosaic, more specifically _cleanup_new_key(). It tests the key (at line 560) for int, but my version is returning the key as a numpy.int64 instead. Any ideas?

rhattersley commented 10 years ago

Hi @ocefpaf - this is looking good. Thanks! (Sorry for the slow response - not sure how I've only just spotted your PR! :expressionless:)

I'll have look at the issues you mention.

Are you OK to sign the CLA available from http://scitools.org.uk/governance.html?

rhattersley commented 10 years ago

my version is returning the key as a numpy.int64 instead. Any ideas?

Python 3 changes int from a fixed-width type to the variable width type. As a result, the NumPy integer array scalars (such as np.int64) no longer inherit from int (see http://docs.scipy.org/doc/numpy/reference/arrays.scalars.html#built-in-scalar-types). So all the checks for isinstance(..., int) need to be changed to reflect that. Perhaps isinstance(..., (int, np.integer))?

rhattersley commented 10 years ago

NB. Once the int/np.integer thing is fixed, attempting to index the result of a zip() will break.

rhattersley commented 10 years ago

I know that some people prefer six, but I think that six is better once the code base is python3 and one wants to make it backwards compatible.

I'm fine with that. This PR doesn't stop us making that change in the future.

ocefpaf commented 10 years ago

Hello @rhattersley,

I have no problem with the CLA. I already signed and emailed according to the instructions.

Thanks for the information on the int issue. I will take a look and fix this PR ASAP.

ocefpaf commented 10 years ago

Fixed index the result of a zip() by making the zip object a list before indexing.
Modified the .travis.yml to build the code so python3 can run nosetests as well (Is ths OK?).
Made the code pep8 compliant.

Do you think it is better if I squash these commits into one?

ocefpaf commented 10 years ago

Hi @rhattersley,

I'm between a rock and a hard place here with travis:

The approach python setup.py build works for python3 but not for python2 (the build for python2 takes place in a different directory!)
Using python setup.py install works for python2 but not for python3 (no idea why, because that works on my local machine).

I'm not used to travis-ci, but I will investigate this further.

rhattersley commented 10 years ago

Hi @ocefpaf - thanks for persevering with this!

To avoid the problems with testing I've added a tweak to use the test_suite option of setuptools. I've also tidied the commits and rebased on to the latest master (so the pep8 tests will pass with the latest version of the pep8 tool) and pushed it all up as a new PR: #69.

So... thanks to your hard work we now have Python 3 support. :tada: Thank you! :smile:

ocefpaf commented 10 years ago

Thanks!