Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add fit_predict to FactorizationMachine #38

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

merrellb
Copy link

@merrellb merrellb commented Feb 8, 2016

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

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

ibayer commented Feb 28, 2016

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)?

@merrellb
Copy link
Author

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?

@ibayer
Copy link
Owner

ibayer commented Apr 19, 2017

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants