-
Notifications
You must be signed in to change notification settings - Fork 105
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
Added more of the sklearn regressors to the presets #27
Conversation
xcessiv/presets/learnersetting.py
Outdated
'sklearn_GP_regressor', | ||
'sklearn_ridge_regressor', | ||
'sklearn_lasso_regressor', | ||
'sklearn_kernelRidge_regressor', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
stylefix: probably better to use sklearn_kernel_ridge_regressor
xcessiv/presets/learnersource.py
Outdated
sklearn_knn_regressor_source = \ | ||
"""from sklearn.neighbors import KNeighborsRegressor | ||
|
||
base_learner = KNeighborsRegressor(random_state=8) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think KNN has a random_state parameter. Please also double check the other estimators to see if they have a random_state parameter.
Looks good so far! Just made a few comments that should be addressed first. |
…equire it, and renamed 'kernelRidge' to 'kernel_ridge'
Think it looks good! Am a bit scared of pulling these without adequate unit tests, but I guess it's my fault for not writing them in the first place. I really need to get to doing those. Thanks a lot! |
no problem :) if you'd like, I can start having a look at writing these tests? |
@enisnazif That would be very helpful! :) |
Added the large majority of the more popular regressors of sklearn. I am aware that a few may be missing. Also, I tidied the code slightly and split the regressors and classifiers into two sections.