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

[MRG] Get the tests passing on Python 3 #105

Closed lesteve closed 7 years ago

lesteve commented 7 years ago

Flake8 fix as well while I was touching the code in the vicinity.

fabianp commented 7 years ago

Great, thanks!

lesteve commented 7 years ago

Actually it looks like this only partially fixes the Python 3 problems ...

fabianp commented 7 years ago

There are some random Cython failures, not sure why ...

lesteve commented 7 years ago

I don't think they are random:

make cython

and in particular

cython --cplus lightning/impl/sgd_fast.pyx

fails pretty consistently both for Python 3 and Python 2.

lesteve commented 7 years ago

I may look at removing the .cpp from the source as we did in scikit-learn when I get around to it. Would you be fine with this?

fabianp commented 7 years ago

I'm fine with that. @mblondel what do you think?

On Tue, Jan 31, 2017 at 4:42 PM, Loïc Estève notifications@github.com wrote:

I may look at removing the .cpp from the source as we did in scikit-learn when I get around to it. Would you be fine with this?

— You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub https://github.com/scikit-learn-contrib/lightning/pull/105#issuecomment-276399097, or mute the thread https://github.com/notifications/unsubscribe-auth/AAQ8hyLmuXsRYtzUHekrmJ0qIPwBup2Bks5rX1ZRgaJpZM4Lyszy .

mblondel commented 7 years ago

That would be great, thanks!

lesteve commented 7 years ago

@fabianp the problem seems to come from https://github.com/scikit-learn-contrib/lightning/pull/97 (expose loss functions in *.pxd file so it can be used by third party modules). I am not a cython expert so I'll let you fix this one. Then I'll have a PR ready for you to remove .cpp files from the repo.

An excerpt of the error when doing: cython --cplus lightning/impl/sgd_fast.pyx

Error compiling Cython file:
------------------------------------------------------------
...
            return 4.0 * y

cdef class Hinge(LossFunction):

    cdef double threshold
               ^
------------------------------------------------------------

lightning/impl/sgd_fast.pyx:59:16: C attributes cannot be added in implementation part of extension type defined in a pxd

More details to show when the problem appeared:

❯ git reset --hard 'f0e74d5^1' && cython --cplus lightning/impl/sgd_fast.pyx
HEAD is now at e1cdcaa Merge pull request #95 from kmike/classes_attribute
❯ git reset --hard 9f5b9b6 && cython --cplus lightning/impl/sgd_fast.pyx
HEAD is now at 9f5b9b6 FIX: remove __init__ from .pxd file

Error compiling Cython file:
------------------------------------------------------------
...
            return 4.0 * y

cdef class Hinge(LossFunction):

    cdef double threshold
               ^
------------------------------------------------------------

lightning/impl/sgd_fast.pyx:59:16: C attributes cannot be added in implementation part of extension type defined in a pxd

Error compiling Cython file:
------------------------------------------------------------
...
            return 4.0 * y

cdef class Hinge(LossFunction):

    cdef double threshold
               ^
------------------------------------------------------------

lightning/impl/sgd_fast.pyx:59:16: 'threshold' redeclared 

Error compiling Cython file:
------------------------------------------------------------
...
        return 0.0

cdef class SmoothHinge(LossFunction):

    cdef double gamma
               ^
------------------------------------------------------------

lightning/impl/sgd_fast.pyx:79:16: C attributes cannot be added in implementation part of extension type defined in a pxd

Error compiling Cython file:
------------------------------------------------------------
...
        return 0.0

cdef class SmoothHinge(LossFunction):

    cdef double gamma
               ^
------------------------------------------------------------

lightning/impl/sgd_fast.pyx:79:16: 'gamma' redeclared 

Error compiling Cython file:
------------------------------------------------------------
...
            return 1 / self.gamma * (1 - z) * y

cdef class SquaredHinge(LossFunction):

    cdef double threshold
               ^
------------------------------------------------------------

lightning/impl/sgd_fast.pyx:109:16: C attributes cannot be added in implementation part of extension type defined in a pxd

Error compiling Cython file:
------------------------------------------------------------
...
            return 1 / self.gamma * (1 - z) * y

cdef class SquaredHinge(LossFunction):

    cdef double threshold
               ^
------------------------------------------------------------

lightning/impl/sgd_fast.pyx:109:16: 'threshold' redeclared 

Error compiling Cython file:
------------------------------------------------------------
...
        return y - p

cdef class Huber(LossFunction):

    cdef double c
               ^
------------------------------------------------------------

lightning/impl/sgd_fast.pyx:159:16: C attributes cannot be added in implementation part of extension type defined in a pxd

Error compiling Cython file:
------------------------------------------------------------
...
        return y - p

cdef class Huber(LossFunction):

    cdef double c
               ^
------------------------------------------------------------

lightning/impl/sgd_fast.pyx:159:16: 'c' redeclared 

Error compiling Cython file:
------------------------------------------------------------
...
            return self.c

cdef class EpsilonInsensitive(LossFunction):

    cdef double epsilon
               ^
------------------------------------------------------------

lightning/impl/sgd_fast.pyx:185:16: C attributes cannot be added in implementation part of extension type defined in a pxd

Error compiling Cython file:
------------------------------------------------------------
...
            return self.c

cdef class EpsilonInsensitive(LossFunction):

    cdef double epsilon
               ^
------------------------------------------------------------

lightning/impl/sgd_fast.pyx:185:16: 'epsilon' redeclared 

Side comment: the commit is not exactly the same as previously because it took you a few tries to manage to get it right.