Skip to content

Fixed python2 libsvm call with unicode kernel#7064

Merged
GaelVaroquaux merged 1 commit intoscikit-learn:masterfrom
Erotemic:quickfix_py2_libsvm_kernel_unicode
Aug 1, 2016
Merged

Fixed python2 libsvm call with unicode kernel#7064
GaelVaroquaux merged 1 commit intoscikit-learn:masterfrom
Erotemic:quickfix_py2_libsvm_kernel_unicode

Conversation

@Erotemic
Copy link
Copy Markdown
Contributor

What does this implement/fix? Explain your changes.

In python2 with from future import absolute_import, division, print_function, unicode_literals I ran across an error when running the line:
When running the line

    from sklearn import svm
    clf = svm.SVC(kernel='linear', C=1)

I received the error:

   TypeError: Argument 'kernel' has incorrect type (expected str, got unicode)

I simply added a check for this case before the offending call, which should prevent it from occurring again.

Any other comments?

@agramfort
Copy link
Copy Markdown
Member

can you add a test?

@Erotemic
Copy link
Copy Markdown
Contributor Author

I added a test. Without my patch to svm/base.py the test will fail. I also extended the commit to ensure that bytes are properly cast to unicode in python3.

@GaelVaroquaux
Copy link
Copy Markdown
Member

Your tests are failing under travis. I am not sure why (master is not failing). Could you rebase on master and force push, to see if that fixes the problem. Thanks!

@lesteve
Copy link
Copy Markdown
Member

lesteve commented Aug 1, 2016

Your tests are failing under travis. I am not sure why (master is not failing). Could you rebase on master and force push, to see if that fixes the problem. Thanks!

I cleared the Travis cache for this PR and this seems to have fixed it.

Added corresponding test.
@Erotemic Erotemic force-pushed the quickfix_py2_libsvm_kernel_unicode branch from 395e9cc to d8db108 Compare August 1, 2016 14:22
@Erotemic
Copy link
Copy Markdown
Contributor Author

Erotemic commented Aug 1, 2016

I went back and rebased. Nothing seemed broken. I also squashed the two separate commits into one.

@GaelVaroquaux
Copy link
Copy Markdown
Member

LGTM! Merging

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.

4 participants