Skip to content

[MRG+1] Fix test_label_binarizer on Windows#6333

Merged
TomDLT merged 2 commits intoscikit-learn:masterfrom
lesteve:fix-test-label-binarizer-on-windows
Feb 17, 2016
Merged

[MRG+1] Fix test_label_binarizer on Windows#6333
TomDLT merged 2 commits intoscikit-learn:masterfrom
lesteve:fix-test-label-binarizer-on-windows

Conversation

@lesteve
Copy link
Copy Markdown
Member

@lesteve lesteve commented Feb 11, 2016

Fix #6280. Looks like .fill is not very friendly with unicode numpy scalar on Windows.

import numpy as np

arr = np.empty(3, dtype='<U3')
unicode_scalar = np.array(['asd'])[0]
arr.fill(unicode_scalar.tolist())  # works with python str
print(arr)

arr.fill(unicode_scalar)  # fills the array with some garbage
print(arr)  # UnicodeDecodeError

This pattern was used here

@lesteve
Fix test_label_binarizer on Windows
6c62763 
Looks like .fill is not very friendly with unicode numpy scalar
@lesteve lesteve changed the title Fix test_label_binarizer on Windows [MRG] Fix test_label_binarizer on Windows Feb 11, 2016
@lesteve
Copy link
Copy Markdown
Member Author

lesteve commented Feb 11, 2016

As #6274 shows, the tests from sklearn/preprocessing/tests are not run on Travis or AppVeyor so the tests should all be green, which doesn't change anything.

Let me know if you want me to incorporate part of #6274 to make sure that test_label_binarizer pass on AppVeyor.

@ogrisel
Copy link
Copy Markdown
Member

ogrisel commented Feb 11, 2016

Let me know if you want me to incorporate part of #6274 to make sure that test_label_binarizer pass on AppVeyor.

That would be great, thanks!

/cc @jakevdp

@lesteve
Copy link
Copy Markdown
Member Author

lesteve commented Feb 11, 2016

test_label_binarizer should be run now. It is easy to check in AppVeyor because nosetests is run in verbose mode. It will probably take a little bit of time before the AppVeyor build runs given the length of the queue though.

@ogrisel ogrisel changed the title [MRG] Fix test_label_binarizer on Windows [MRG+1] Fix test_label_binarizer on Windows Feb 11, 2016
@ogrisel
Copy link
Copy Markdown
Member

ogrisel commented Feb 11, 2016

+1 once appveyor is green.

@lesteve
Copy link
Copy Markdown
Member Author

lesteve commented Feb 11, 2016

AppVeyor build completed and you can see test_label_binarizer is run indeed for example here.

Just need another +1 now.

@TomDLT
Copy link
Copy Markdown
Member

TomDLT commented Feb 17, 2016

LGTM

TomDLT added a commit that referenced this pull request Feb 17, 2016
@TomDLT
Merge pull request #6333 from lesteve/fix-test-label-binarizer-on-win…
ffafc80 
…dows

[MRG+1] Fix test_label_binarizer on Windows
@TomDLT TomDLT merged commit ffafc80 into scikit-learn:master Feb 17, 2016
@lesteve lesteve deleted the fix-test-label-binarizer-on-windows branch February 17, 2016 14:34
@lesteve lesteve mentioned this pull request Feb 17, 2016
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

Waiting for Reviewer

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@lesteve @ogrisel @TomDLT