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.

Sign up for GitHub

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

Jump to bottom

Learner.model_params will return weights with the wrong sign if sklearn is fixed #111

Closed
mheilman opened this issue Mar 4, 2014 · 3 comments
Closed

Learner.model_params will return weights with the wrong sign if sklearn is fixed #111

mheilman opened this issue Mar 4, 2014 · 3 comments
Labels
sklearn-compatibility
Milestone
1.1

Comments

@mheilman
Copy link
Contributor

mheilman commented Mar 4, 2014

Currently, sklearn appears to output coefficients with the wrong sign for SVR. See scikit-learn/scikit-learn#2933.

SKLL pull request #110 adds support to SKLL for printing SVR model weights and corrects the sign. However, whenever sklearn is fixed, the correction should be removed.
@dan-blanchard
Copy link
Contributor

It looks like they've fixed scikit-learn/scikit-learn#2933, but it's not released yet. We should keep in mind that whenever the next release of scikit-learn comes out, we'll have to fix this.

@dan-blanchard dan-blanchard added this to the 1.1 milestone Apr 6, 2015
@dan-blanchard
Copy link
Contributor

This is in 0.16, so we need to fix it before next release.

desilinguist added a commit that referenced this issue Jul 11, 2015
@desilinguist
Update learner.py
162f93c 
- Remove SVR coefficient correction (#111) now that the original bug has been fixed in sklearn.
- Remove dependency on `BaseLibLinear` in `model_params()` since it is no longer exposed in scikit-learn.
- Expose `LinearSVR` from scikit-learn and so do not have SVR have a 'linear' kernel by default.
- Include `RescaledLinearSVR` among the rescaled regressors.
- Remove `SVR` from `test_linear_models()` in `test_regression.py` and create a new `test_non_linear_models()`.
@desilinguist desilinguist mentioned this issue Jul 11, 2015
Merged
@desilinguist
Copy link
Member

Addressed by #243.

desilinguist added a commit that referenced this issue Jul 19, 2015
@desilinguist @dan-blanchard
Update learner.py
8580a99 
- Remove SVR coefficient correction (#111) now that the original bug has been fixed in sklearn.
- Remove dependency on `BaseLibLinear` in `model_params()` since it is no longer exposed in scikit-learn.
- Expose `LinearSVR` from scikit-learn and so do not have SVR have a 'linear' kernel by default.
- Include `RescaledLinearSVR` among the rescaled regressors.
- Remove `SVR` from `test_linear_models()` in `test_regression.py` and create a new `test_non_linear_models()`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
sklearn-compatibility
Projects
None yet
Milestone
1.1
Development

No branches or pull requests
3 participants
@desilinguist @mheilman @dan-blanchard