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

Improve SVM hyperparameters #2651

Merged
merged 8 commits into from
Aug 20, 2021
Merged

Improve SVM hyperparameters #2651

merged 8 commits into from
Aug 20, 2021

Conversation

eccabay
Copy link
Contributor

@eccabay eccabay commented Aug 18, 2021

Closes #2615 by removing "linear" as an option from SVMClassifier and SVMRegressor, and swaps "auto" to be SVM's default gamma parameter for increased first-guess performance (as discussed in performance test results here)

@codecov
Copy link

codecov bot commented Aug 18, 2021

Codecov Report

Merging #2651 (1256989) into main (ed74174) will not change coverage.
The diff coverage is 100.0%.

Impacted file tree graph

@@          Coverage Diff          @@
##            main   #2651   +/-   ##
=====================================
  Coverage   99.9%   99.9%           
=====================================
  Files        298     298           
  Lines      27305   27305           
=====================================
  Hits       27261   27261           
  Misses        44      44           
Impacted Files Coverage Δ
...omponents/estimators/classifiers/svm_classifier.py 100.0% <ø> (ø)
evalml/tests/component_tests/test_components.py 100.0% <ø> (ø)
.../components/estimators/regressors/svm_regressor.py 100.0% <100.0%> (ø)
...valml/tests/component_tests/test_svm_classifier.py 100.0% <100.0%> (ø)
evalml/tests/component_tests/test_svm_regressor.py 100.0% <100.0%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ed74174...1256989. Read the comment docs.

@eccabay eccabay self-assigned this Aug 18, 2021
@eccabay eccabay marked this pull request as ready for review August 18, 2021 14:56
Copy link
Contributor

@chukarsten chukarsten left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! Just two hyper nits about the doc strings!

Copy link
Contributor

@freddyaboulton freddyaboulton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@eccabay Thank you for this! I agree with removing the linear kernel for both regression and classification but not sure if we should tweak the other hyperparameters for regression since the perf tests you only ran on binary classification.

I left a comment on your results as well about how much better SVM is than the next best estimator. The fit time is only slightly slower for most datasets but for some it's like 4x slower. If the SVM more than 4x better, there is an argument for including it in AutoMLSearch.

I'd like to continue the discussion on your perf test doc before approving!

@@ -42,7 +42,7 @@ class SVMRegressor(Estimator):
ProblemTypes.TIME_SERIES_REGRESSION,
]"""

def __init__(self, C=1.0, kernel="rbf", gamma="scale", random_seed=0, **kwargs):
def __init__(self, C=1.0, kernel="rbf", gamma="auto", random_seed=0, **kwargs):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we make this change? The perf tests only considered binary classification problems.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a fair point! It's hard to say since we don't have very many regression datasets in looking glass. I'll run a few tests with what we have and see if the results are consistent or not!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@freddyaboulton Results from the regression testing is now in the performance test doc! On the very small number of datasets we have, "auto" performs better significantly more often than "scale", so I think this change should happen.

@eccabay
Copy link
Contributor Author

eccabay commented Aug 18, 2021

One thing I forgot to implement/mention before publishing this PR is that kernel=precomputed also needs to be removed - I had it removed in my testing branch and forgot to carry it over into this one. With this parameter, Sklearn's SVC expects a precomputed kernel of shape (n_samples_test, n_samples_train), which we don't give it, so it errors out and doesn't run in the first place.

Copy link
Contributor

@bchen1116 bchen1116 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Also curious about the change of gamma from scale to auto, but will wait for the results on that!

Copy link
Contributor

@freddyaboulton freddyaboulton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @eccabay !

@eccabay eccabay merged commit 406dd6b into main Aug 20, 2021
@eccabay eccabay deleted the svm_hyperparameters branch August 23, 2021 12:17
@chukarsten chukarsten mentioned this pull request Sep 1, 2021
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.

Handle scaling for SVM
4 participants