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

BUG: Compare strings for equality, not identity. #2558

Merged
merged 1 commit into from
Oct 30, 2013
Merged

BUG: Compare strings for equality, not identity. #2558

merged 1 commit into from
Oct 30, 2013
+2 −2

Conversation

Jorge-C
Copy link
Contributor

@Jorge-C Jorge-C commented Oct 30, 2013

In a script I found that calling MDS with dissimilarity='precomputed' was failing because strings are compared by identity. A quick way to reproduce the bug is this:

from sklearn.manifold import MDS
import numpy as np

mds = MDS(dissimilarity=''.join(list('precomputed')))
X = np.ones(10,10)
pos = mds.fit_transform(X)

raises a

ValueError: Proximity must be 'precomputed' or 'euclidean'. Got precomputed instead

The fix is really simple, but I can add a test case to it or an issue if you prefer that.

@Jorge-C
BUG: Compare strings for equality, not identity.
1564765 
Strings in Python are usually interned (https://en.wikipedia.org/wiki/String_interning) but not always, so comparing identity can fail.
@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling 1564765 on Jorge-C:patch-1 into 30eb78d on scikit-learn:master.

mblondel added a commit that referenced this pull request Oct 30, 2013
@mblondel
Merge pull request #2558 from Jorge-C/patch-1
03926cc 
BUG: Compare strings for equality, not identity.
@mblondel mblondel merged commit 03926cc into scikit-learn:master Oct 30, 2013
@mblondel
Copy link
Member

Merged, thanks! :-)

@Jorge-C Jorge-C deleted the patch-1 branch October 30, 2013 15:55
@Jorge-C Jorge-C restored the patch-1 branch October 30, 2013 15:55
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
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Jorge-C @coveralls @mblondel