Skip to content

Attempt porting to python 3.#25

Open
bluescarni wants to merge 1 commit intocbecker:masterfrom
bluescarni:python3
Open

Attempt porting to python 3.#25
bluescarni wants to merge 1 commit intocbecker:masterfrom
bluescarni:python3

Conversation

@bluescarni
Copy link
Copy Markdown

This came up as part of the work on the conda package for iiboost. It imports fine on python3, but I was not able to test that it really works because the tests cannot be run as they rely on data pickled on python2 that apparently cannot be loaded from python3.

@@ -1 +1,4 @@
from booster import Booster, EigenVectorsOfHessianImage, computeEigenVectorsOfHessianImage, computeIntegralImage, ROICoordinates
# for python 2.0 compatibility
from __future__ import absolute_import as _ai
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why the aliasing?

Comment thread python/CMakeLists.txt
# IF(NOT (${PYTHON_MAJOR_VERSION} EQUAL 2))
# MESSAGE(FATAL_ERROR "Python bindings require Python 2.x.")
# ENDIF()
#ENDIF()
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess this can just be dropped, right?

Comment thread python/CMakeLists.txt
# OUTPUT_VARIABLE PYTHON_LIBRARY_NAME OUTPUT_STRIP_TRAILING_WHITESPACE)
# FIND_LIBRARY(PYTHON_LIBRARIES ${PYTHON_LIBRARY_NAME} HINTS "${PYTHON_PREFIX}"
# PATH_SUFFIXES lib lib64 libs DOC "Python libraries")
#ENDIF()
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same story regarding dropping.

Copy link
Copy Markdown

@jakirkham jakirkham May 23, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should add that while I haven't built this package before, I have had some rough experiences with CMake finding the right Python. If this works, great, but it might be worth checking with others who added this to verify it still works.

Comment thread python/iiboost/booster.py

from exceptions import RuntimeError
if sys.version_info[0] < 3:
from exceptions import RuntimeError
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this even need to be imported at all? FWICT this is already available on Python 2.7 and 2.6 without importing.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also might be worth adding from __future__ import print_function here as well.


# load data
print "--- Loading data ---"
print("--- Loading data ---")
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might be worth adding from __future__ import print_function as the first line above to make sure this behavior is maintained even on Python 2.

@cbecker
Copy link
Copy Markdown
Owner

cbecker commented May 24, 2017

Thanks both for the comments and PR.

I haven't tried this yet, but is it now working with both python 2 and python 3?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants