Navigation Menu

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

Update classifier.py #176

Merged
merged 2 commits into from Aug 7, 2016
Merged

Update classifier.py #176

merged 2 commits into from Aug 7, 2016

Conversation

wangg12
Copy link
Contributor

@wangg12 wangg12 commented Aug 5, 2016

What does this PR do?

To improve the classifier demo by adding an option to train with grid search svm.
Mostly copy paste from the web demo.

Where should the reviewer start?

demos/classifier.py

How should this PR be tested?

test the simple classifier.py demo

Any background context you want to provide?

What are the relevant issues?

[You can link directly to issues by entering # then the
number of the issue, for example, #3 links to issue 3]

Screenshots (if appropriate)

Questions:

  • Do the docs need to be updated?
  • Does this PR add new (Python) dependencies?
    No
    Add an option to train with grid search svm.

Add an option to train with grid search svm.
@bamos
Copy link
Collaborator

bamos commented Aug 5, 2016

Hi @wangg12 - do you see better performance with the grid search over just a linear SVM with C=1? I used to use the grid search as the default everywhere, but the performance was marginally better than just using a linear SVM and wasn't worth the extra time the grid search took.

I'd be okay adding this since it's just another option. Can you make the following minor changes to your PR:

  • Replace print 'training with grid search svm' with the following and make sure your modifications here don't cause flake8/pep8 warnings that will fail the Travis build.
print("""
Warning: In our experiences, using a grid search over SVM hyper-parameters only
gives marginally better performance than a linear SVM with C=1 and
is not worth the extra computations of performing a grid search.
""")
  • Import GridSearchCV at the top

@bamos bamos merged commit 80014e0 into cmusatyalab:master Aug 7, 2016
@bamos
Copy link
Collaborator

bamos commented Aug 7, 2016

Thanks, merged. By the way, I don't get notifications if you update a PR and don't comment on it.

@wangg12
Copy link
Contributor Author

wangg12 commented Aug 7, 2016

Thanks, I forgot to comment on it.

nhzandi pushed a commit to nhzandi/openface that referenced this pull request Mar 28, 2017
* Update classifier.py

Add an option to train with grid search svm.

* Update classifier.py
sunnylgz pushed a commit to sunnylgz/openface that referenced this pull request Dec 28, 2017
* Update classifier.py

Add an option to train with grid search svm.

* Update classifier.py
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.

None yet

2 participants