-
Notifications
You must be signed in to change notification settings - Fork 509
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.
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
Add python3 support #196
Add python3 support #196
Conversation
…2 and py3 Six provides simple utilities for wrapping over differences between Python 2 and Python 3. It is intended to support codebases that work on both Python 2 and 3 without modification. six consists of only one Python file, so it is painless to copy into a project. See https://pythonhosted.org/six/
six.string_types represents the possible types for text data. This is basestring() in Python 2 and str in Python 3.
This was done with the help of 2to3 refactoring tool and the addition of "from __future__ import print_function" where appropriate: $ 2to3 -f print . # dry-run $ 2to3 -f print -w . # proceed to changes
From six documentation [1]: Get the next item of iterator it. StopIteration is raised if the iterator is exhausted. This is a replacement for calling it.next() in Python 2 and next(it) in Python 3. [1] https://pythonhosted.org/six/#object-model-compatibility
From six documentation [1]: Returns an iterator over dictionary‘s items. This replaces dictionary.iteritems() on Python 2 and dictionary.items() on Python 3. kwargs are passed through to the underlying method. [1] https://pythonhosted.org/six/#object-model-compatibility
…ion. The "cmp" builtin function has been removed from py3 series. See [1] [1] https://docs.python.org/3.0/whatsnew/3.0.html#ordering-comparisons
To facilitate the update of the notebooks, a convenient script named "fix_notebook_print.py" has been used. Then, import of print_function was manually added. $ ack "print " -l bin/Notebooks/helloRadiomics.ipynb bin/Notebooks/helloFeatureClass.ipynb bin/Notebooks/PyRadiomics Example.ipynb $ /tmp/fix_ipython_notebook_print.py ./bin/Notebooks/helloRadiomics.ipynb bin/Notebooks/helloFeatureClass.ipynb bin/Notebooks/PyRadiomics\ Example.ipynb Fixing ./bin/Notebooks/helloRadiomics.ipynb - done (9 lines updated) Fixing bin/Notebooks/helloFeatureClass.ipynb - done (37 lines updated) Fixing bin/Notebooks/PyRadiomics Example.ipynb - done (25 lines updated) Script available here: https://gist.github.com/jcfr/7366dda36f86deaea2d81ffee160bf3f
Update of the code was facilitated by the use of the 2to3 refactoring tool: $ 2to3 -f dict . # dry-un $ 2to3 -f dict -w . # recactor Updates where code was (1) iterating of the keys values or (2) getting the number of keys wasn't updates. Co-authored-by: Joost van Griethuysen <[email protected]>
This commit fixes the following error reported when running the tests using python3: File "/home/jcfr/Projects/pyradiomics/tests/testUtils.py", line 184, in readBaselineFiles headers = six.next(csvReader) _csv.Error: iterator should return strings, not bytes (did you open the file in text mode?)
With newer versions of SimpleITK and python, a tuple or list is expected. Direcly passing a numpy array is not supported. This commit fixes error like the following: File "/home/jcfr/Projects/pyradiomics/radiomics/imageoperations.py", line 125, in cropToTumorMask cif.SetLowerBoundaryCropSize(ijkMinBounds) File "/home/jcfr/.virtualenvs/pyradiomics-py3/lib/python3.5/site-packages/SimpleITK/SimpleITK.py", line 21397, in SetLowerBoundaryCropSize return _SimpleITK.CropImageFilter_SetLowerBoundaryCropSize(self, LowerBoundaryCropSize) TypeError: in method 'CropImageFilter_SetLowerBoundaryCropSize', argument 2 of type 'std::vector< unsigned int,std::allocator< unsigned int > > const &' Co-authored-by: Jean-Christophe Fillion-Robin <[email protected]>
In python 3.5 GLRLM results differed from baseline. This was due to the different implementation of the `filter()` function, which returns an iterator instead of a list. In GLRLM calculation, there where 2 iterations over this iter-object (one for flat-angle check, one for matrix calculation), causing it to break in python 3.5. Fix by merging the 2 iterations into one loop. Additionally, simplify the filter function.
…str" Some functions expect string types (such as in nose-testing), but python 3.5 by default provides unicode strings. This commit casts where necessary. Also change csv.writer mode from 'wb' to 'w', which accepts regular strings.
Since in Python3, zip() statement returns an iterator, this commit use the list() statement to exhaust the iterator and initialize the numpy array. Co-authored-by: Jean-Christophe Fillion-Robin <[email protected]>
Since no MacOSX SimpleITK wheels for Python 3.x is available, corresponding Travis builds havn't been enabled.
.circleci-matrix.yml
Outdated
# - MANYLINUX_IMAGE=manylinux-x64 MANYLINUX_PYTHON=cp35-cp35m | ||
# - MANYLINUX_IMAGE=manylinux-x86 MANYLINUX_PYTHON=cp35-cp35m | ||
- MANYLINUX_IMAGE=manylinux-x64 MANYLINUX_PYTHON=cp33-cp33m | ||
- MANYLINUX_IMAGE=manylinux-x86 MANYLINUX_PYTHON=cp33-cp33m |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These two lines cause CircleCI to fail, as it tries to install numpy == 1.12.0, which does not support python 3.3 anymore.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed, I am now looking into adding few env. markers
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thinking more about it, as you suggested let's just remove support for Python 3.3. It is now worth the complication
9df015d
to
7ea5480
Compare
If really needed, we could look into specifying some more complex requirements using Environment Markers [1] [1] https://www.python.org/dev/peps/pep-0496/ This commit avoids to get the following error: Collecting numpy>=1.9.2 (from -r requirements.txt (line 1)) Downloading numpy-1.12.0.zip (4.8MB) 100% |████████████████████████████████| 4.8MB 240kB/s Complete output from command python setup.py egg_info: Traceback (most recent call last): File "<string>", line 1, in <module> File "/tmp/pip-build-7w8sbj/numpy/setup.py", line 34, in <module> raise RuntimeError("Python version 2.7 or >= 3.4 required.") RuntimeError: Python version 2.7 or >= 3.4 required. Reported-by: Joost van Griethuysen <[email protected]>
7ea5480
to
2a25157
Compare
A new commit addressing the concern has been pushed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Additionally, the variable in_py3
can be removed from __init__.py
, as this is replaced by six.PY3
@@ -19,11 +19,6 @@ matrix: | |||
# env: | |||
# - PYTHON_VERSION=3.4.5 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't this mean that python 3.x testing for Mac is still disabled?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
python 3.x testing for Mac is still disabled?
Indeed. Waiting for SimpleITK wheels, we do not have a choice. See https://itk.org/SimpleITKDoxygen/html/PyDownloadPage.html
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are however conda packages for SimpleITK, would it make sense to get the mac tests working on miniconda/anaconda (https://anaconda.org/simpleitk/simpleitk) instead of standard python?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jcfr ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That would make sense, the idea would be to submit a recipe to conda-forge. With that in place, the package would be available on conda on linux, macosx and windows.
That said, conda-forge recipe are updated only when there is a release of the package. If you would like to setup continuous integration using miniconda, you would have to set it up on your own.
I currently do not have the bandwidth to set miniconda CI up for pyradiomics, in the mean time I would be happy to review PR.
@blowekamp Do we know when macosx wheels will be available for python 35 ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was just working on some scripts for the 1.0 release... I'll see if I can run them on the release branch too like the recent x86_64 0.10.0.post3 wheel.
Suggested-by: Joost van Griethuysen <[email protected]>
@jcfr, last build of CircleCI failed, python 3.x couldn't find |
I noticed. I will be looking at this later today. |
I identified the problem and submitted a fix to upstream setuptools. See pypa/setuptools#971 In the mean time, I will add a workaround. |
f38134a
to
fc3f1b7
Compare
…ptools issue Since: (1) CircleCI automatically adds a symlink from /home/ubuntu/pyradiomics/venv to /home/ubuntu/virtualenvs/venv-system, (indeed it detects it is a python project) (2) the symlink is indeed invalid when "seen" from within the docker container building the project (because it mount the current folder into the dockcross/manylinux-x64 image), (3) distutils available in python 3.4.6 has an issue causing sdist to fail when it find an invalid symlink (http://bugs.python.org/issue12885) Setuptools is also failing to monkey patch for that version of python. A patch has been submitted upstream: pypa/setuptools#971 To workaround the issue, we simply remove the symbolic link and avoid the problem completely.
fc3f1b7
to
de8ee88
Compare
Et voila. As soon as, Travis returns green. Will be integrating 🎆 |
All comments have been addressed. CircleCI is now green :)
This set of changes supersedes #195.
It includes changes required to run pyradiomics on Python 3. To gracefully handles the different between py2 and py3, it introduces the dependency on
six
. See https://pythonhosted.org/six/Notes:
dict.items()
(which is less efficient in py2), it uses six.iteritems