epfml / sent2vec

General purpose unsupervised sentence representations
Other
1.19k stars 256 forks source link

Import standard int64_t instead of ctypedef'ing it #72

Closed vsolovyov closed 5 years ago

vsolovyov commented 5 years ago

When I tried to compile last version on OS X 10.14 (Python 3.7.0, Cython 0.29.7), I encountered this error:

> python setup.py bdist
...
src/sent2vec.cpp:3932:15: error: no matching function for call to '__pyx_convert_vector_to_py___pyx_t_8sent2vec_int64_t'
  __pyx_t_2 = __pyx_convert_vector_to_py___pyx_t_8sent2vec_int64_t(__pyx_v_self->_thisptr->getUnigramsCounts()); if (unlikely(!__pyx_t_2)) __PYX_ERR(0, 114, __pyx_L1_error)
              ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
src/sent2vec.cpp:2000:18: note: candidate function not viable: no known conversion from 'vector<int64_t>' to 'const vector<__pyx_t_8sent2vec_int64_t>' for 1st argument
static PyObject *__pyx_convert_vector_to_py___pyx_t_8sent2vec_int64_t(const std::vector<__pyx_t_8sent2vec_int64_t>  &); /*proto*/
                 ^
1 error generated.
error: command 'clang' failed with exit status 1

The code where the problem occured looked as simple as it gets, just a simple conversion of vector<int64_t> to Python list. However, the c++ function name was somewhat suspicious, and so did the type __pyx_t_8sent2vec_int64_t. Why the type of an integer had a sent2vec in it? Turns out, because int64_t was defined in sent2vec.pyx. And somehow compiler on Linux could figure out that it's the same as standard, and on OSX it didn't.

So I just changed ctypedef to a proper import of a standard type and the file compiled.

Also, to get it to link I had to remove rt library in setup.py for python library, and in Makefile for a binary. If you want, I can create a PR that removes that library if the build happens on OSX.

mpagli commented 5 years ago

If you want, I can create a PR that removes that library if the build happens on OSX.

This would be really nice. I can then run some tests and merge.

mpagli commented 5 years ago

LGTM