lucastheis / cmt

Fast CPU implementations of several conditional probabilistic models
http://lucastheis.github.io/cmt/
MIT License
36 stars 14 forks source link

Support Python 3 #28

Open jakirkham opened 7 years ago

jakirkham commented 7 years ago
FlorianFranzen commented 7 years ago

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.

jakirkham commented 7 years ago

I am not sure why 3decc40 is included, as it has been merged with #30.

This was created before that PR was merged and has simply not been updated. Was trying to get rid of already fixed test failures while debugging. Will rebase it out.

jakirkham commented 7 years ago

Have rebased.

@lucastheis Can you take a closer look at 771706f and a8290de.

As a consequence, these have changed to ( https://github.com/lucastheis/cmt/commit/6842b96a93c7e9af238c3fa3f6a3ccb913ace260 ) and ( https://github.com/lucastheis/cmt/commit/bdfb406df4ce83d68c4028395bdf9d99fca78512 ) respectively. Though their content should be the same.

jakirkham commented 7 years ago

One thing does concern me with this Python 3 support, am seeing a segfault pretty reliably in the tools_test.py, which doesn't occur on Python 2.

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 gotos to keep all the exception propagating code in one place (normally at the end of the function after the last return).

C Python Exception Handling: Any time a Python C API function returns a PyObject* or other pointer, one should be checking to see if it is NULL. Alternatively if an int is returned, one should be checking to see if it is -1. When one of these respective situations occurs, one must handle the exception. This means either clearing it somehow and doing some workaround or returning NULL/-1 respectively. Note there is no need to set the exception when propagating the error as it is already set. If the exception is not handled, bad things usually happen (e.g. segfaults). Also if NULL or -1 get passed to other Python C API functions or returned by any functions here, other strange things can happen (e.g. again segfaults). More details about this can be seen in the docs of Python 2 and Python 3

lucastheis commented 7 years ago

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.