search

ibayer / fastFM

fastFM: A Library for Factorization Machines
http://ibayer.github.io/fastFM
Other
1.08k stars 204 forks source link

Feature add fit function #121

Closed AlexJoz closed 6 years ago

AlexJoz commented 6 years ago

@ibayer, can You please take a quick look? Just to know i'm moving to the right direction.. fit returns only 'nan's now=(

ibayer commented 6 years ago

fit returns only 'nan's now=(

I'm not surprised, we are not yet passing any settings to the c++ solver (I think this functionality is still missing from the c++ API, see https://github.com/ibayer/fastFM-core2/issues/49).

ibayer commented 6 years ago

Btw debugging the cython layer is a bit tricky, but using a lot of logging output in the C++ code (to see if input data get's properly wrapped, functions get called etd..) helped me a lot. https://github.com/ibayer/fastFM-core2/issues/22

AlexJoz commented 6 years ago

Just played a bit.. And here is my question: should we implement serialization within this feature PR?

ibayer commented 6 years ago

And here is my question: should we implement serialization within this feature PR?

Good questions, I would say no (but you can probably estimate the complexity better). In this PR we could just aim to map the data properly and get fit to do some calculation. We can hard code the required settings for the solver on the c++ side and not pass any settings for now.

Passing specific settings from python to c++ can then be addressed in a separate PR.

AlexJoz commented 6 years ago

@ibayer ok, rolled back with serialization. But i'm a bit stuck. In model interface we have only add_param methods, and also we don't use random weight initialization, so if i force to use zeros params, the fit func gives me all nan (as we cannot divide by zero). The other functionality with fit function wrapper seems to work fine. Can you give me the hint, if i'm wrong.. and where should I move next =)

ibayer commented 6 years ago

@AlexJoz Please keep the serialization stuff around (maybe different branch, we can use it for other PR).

I'm not quit sure I understand the issue with the zero initialization. Only V requires random init and we can do this on the python side for now (in the test code, or temporally added to the fit function).

ibayer commented 6 years ago

Looks really good what you are doing. :+1:

ibayer commented 6 years ago

Should I merge?

AlexJoz commented 6 years ago

@ibayer, You are the commander ; )

ibayer commented 6 years ago

Thanks for your work!