Support Python 3 #28
Support Python 3 #28
Conversation
jakirkham
force-pushed
the
ci_python_3
branch
3 times, most recently
from
60a2152 to
d8860c8
Compare
jakirkham
force-pushed
the
ci_python_3
branch
2 times, most recently
from
3c799f8 to
771706f
Compare
|
Looks good. Thank you for your work and the detailed commit log. I am not sure why 3decc40 is included, as it has been merged with #30. @lucastheis Can you take a closer look at 771706f and a8290de. |
These won't work with Python 3. So simply drop this configuration from Travis CI.
As these will be Python 2 packages, we will need to do without them. Can get them as wheels from PyPI or conda packages to avoid spending time building them.
As we are no longer installing with `sudo`, disable it to get faster CI startup time from Travis CI.
Forces installation from pre-built binaries (wheels).
This appears to be an annoying bug that was introduced in the very beginning of Python 3 and has been around ever since unfortunately. :( The proposed workaround is to just use note a method can also handle varargs. Though it looks like it is getting fixed as of this year. I don't think we will have releases generally available for this fix in the near term. So this patch should suffice. More details in the linked references. ref: https://bugs.python.org/issue11587 (original) ref: https://bugs.python.org/issue15657 (duplicate, was fixed)
It seems this function should be taking arguments. At least its usage seems to and definition seem to indicate that. So set its argument format to `METH_VARARGS`.
It seem the default version of pip that Travis CI is using on Python 2.7 is quite ancient (version 6.0.7). To remedy this, go ahead and upgrade pip before doing anything else. As a precautionary measure, run pip through Python when doing the upgrade. While calling pip directly to perform this upgrade shouldn't cause any issues, some have reported problems on Windows. Though these cases are likely due to how file removal is handled on Windows. In any event, taking the safest route possible seems wise when upgrading pip.
NumPy is deprecating the use of `-` on NumPy arrays. So make use of the suggested `^`, which will give the same result.
On Python 3, the default string is of Unicode type, which caused this comparison some issues. In particular, the length comparison was off as the Unicode string may have more bytes than the equivalent ASCII string. To fix that, just encode Unicode strings as ASCII byte strings in Python. This way handling of them can proceed exactly as before. As it is also possible to get Unicode values on Python 2, it doesn't hurt to handle them appropriately as well.
In some cases a conversion may not go smoothly or may not be the expected type. The return values in these cases will be `NULL`. If an error is set twice though, this result in an interpreter crash. So try to handle a few of these around string conversion to avoid crashing. Also call `PyErr_Clear` before throwing exceptions where there may already be a Python exception being unwound. If there is not one being unwound, this is a no-op.
In Python 3, `range` behaves like `xrange`. So it does not eagerly evaluate and return a `list`. Instead it remains an object that can be iterated over and has a few other properties. This differs from Python 2 where `range` returns a `list`. To correct this incompatibility, simply pass the result of `range` to a `list`. If the result is already a `list` as it is on Python 2, this is a no-op. However if `range` returns an iterable object like it is on Python 3, this will convert it to a `list` giving the expected result seen on Python 2.
It appears that SciPy's `ks_2samp` has slightly different behavior on Python 2 vs. Python 3. In particular on Python 2 it reduces to its `pvalue` when performing comparisons to numeric values. However on Python 3 this behavior does not appear to be supported. To address this issue, simply get the `pvalue` attribute regardless. This yields the correct behavior on both Python 2 and Python 3. More details in the linked issue. xref: scipy/scipy#6099
|
Have rebased.
As a consequence, these have changed to ( 6842b96 ) and ( bdfb406 ) respectively. Though their content should be the same. |
|
One thing does concern me with this Python 3 support, am seeing a segfault pretty reliably in the I'm not entirely sure what is causing it. That said, I did notice a lot of the C++ code was not properly checking Python exceptions and handling them (see last paragraph). It is possible this segmentation fault is some Python 2/3 incompatibility that would have raised an exception, but was not properly handled. So it comes back as a segfault instead. Unfortunately this is not likely something I'm going to have time to debug/fix. That said, I have been seeing this segfault crop up since I started this PR, which suggests that this was already an issue in the existing code. Also the aforementioned commits do show how one goes about handling these sorts of exceptions. The NumPy codebase also has some really good examples. Often people like to use C Python Exception Handling: Any time a Python C API function returns a |
|
Thanks for all your work. Looks good on first glance. I will need a bit more time to understand why some tests fail before merging. |
printfunction with Python for 2/3 compatibility.input_indicesfunction.^with NumPy bool arrays instead of-.adjust_gradientinPreconditioner#30 ).