fastFM icon indicating copy to clipboard operation
fastFM copied to clipboard

Add fit_predict to FactorizationMachine

Open merrellb opened this issue 9 years ago • 3 comments

Although required (and now overridden) by mcmc, having this method always present makes it easier to test multiple solvers with minimal code changes

merrellb avatar Feb 08 '16 15:02 merrellb

I don't like to change the API except for a very good reason, especial if this moves us further away for the sklearn API. I'm not sure if easier to test qualifies as a strong enough reason. What do you think? Can you give an example where having fit_predict for sgd is better then fit(X,y).predict(X_test)?

ibayer avatar Feb 28 '16 11:02 ibayer

I am not sure I would say this is changing the API but rather making it consistent. Once fit_predict has been added to the API for MCMC I think it is confusing to not have it available for SGD and ALS which can support it just as easily. This is in line with Python's duck typing which discourages needing to test for type before calling a method for similar objects. This additionally seems consistent with sklearn which has fit(), transform() and fit_transform() for most classes (even if fit_transform() is not needed or provides any advantage). If sklearn compatibility is a goal perhaps their terminology would be better?

merrellb avatar Feb 28 '16 16:02 merrellb

I just revisit this PR once again but still I can't make up my mind if replacing fit_predict by fit_transform is the right way to go. It improves consistency (I would be fine with adding it to every model) and compatibility with sklearn. However the semantic is a bit confusing, that the reason why I went with fit_predict in the first place.

ibayer avatar Apr 19 '17 19:04 ibayer