scikit-learn-contrib / lightning

Large-scale linear classification, regression and ranking in Python
https://contrib.scikit-learn.org/lightning/
1.73k stars 214 forks source link

change return values of Penalty function in sag_fast #107

Open casotto opened 7 years ago

casotto commented 7 years ago

Hello, I am using the Penalty class in sag_fast.pyx and I need to propagate an exception which occurs inside the projection method. Since the return value of the function is void, cython is not able to propagate this error to the layers above, as explained here. The solution they propose is to change the return value to int and add the except -1 in the declaration.

I wonder if it would be ok to change

 cdef void projection(self,
                         double* w,
                         int* indices,
                         double stepsize,
                         int n_nz):

to

 cdef int projection(self,
                         double* w,
                         int* indices,
                         double stepsize,
                         int n_nz) except -1:

At the moment I am not able to test this change. This is due the fact that I am working on a Windows environement, and as the setup.py compiles the .cpp cython-compiled files, that is something I am not able to do in my computer (even though it works in appveyor). The best way would be to remove the cpp files and force at the setup the compilation of the extension by cython, as you already proposed.

If you are ok with the type changement, I will send a pull request whenever I am able to use linux where the package compiles correctly

fabianp commented 7 years ago

Hi @casotto ! Thanks for the report. The solution sounds good. If you want you can make a pull request with the fix and I will re-generate the .cpp files.