8000 [MRG+2] convert to boolean arrays for boolean distances (ex: jaccard) by TomDLT · Pull Request #5460 · scikit-learn/scikit-learn · GitHub
[go: up one dir, main page]
Skip to content

[MRG+2] convert to boolean arrays for boolean distances (ex: jaccard) #5460

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
Closed
wants to merge 1 commit into from
Closed

[MRG+2] convert to boolean arrays for boolean distances (ex: jaccard) #5460

wants to merge 1 commit into from

Conversation

TomDLT
Copy link
Member
@TomDLT TomDLT commented Oct 19, 2015 •
edited
Loading

amueller
amueller reviewed Oct 21, 2015
View reviewed changes
sklearn/metrics/pairwise.py
@@ -1195,12 +1196,19 @@ def pairwise_distances(X, Y=None, metric="euclidean", n_jobs=1, **kwds):
" support sparse matrices.")
X, Y = check_pairwise_arrays(X, Y)
if n_jobs == 1 and X is Y:
if metric in PAIRWISE_BOOLEAN_FUNCTIONS and X.dtype != bool:
warnings.warn("Metric %s: Implicit cast to boolean for X of "
Copy link
Member

Choose a reason for hiding this comment

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

DataConversionWarning?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure!

@jakevdp
Copy link
Member
jakevdp commented Oct 21, 2015

Looks good. We'd want some regression tests that check the behavior for non-boolean inputs.

@TomDLT TomDLT changed the title [WIP] jaccard distance [MRG] jaccard distance Oct 22, 2015
@TomDLT TomDLT changed the title [MRG] jaccard distance [MRG] convert to boolean arrays for boolean distances (ex: jaccard) Oct 22, 2015
@TomDLT
Copy link
Member Author
TomDLT commented Oct 22, 2015

Tests added, that fail on master

@jakevdp
Copy link
Member
jakevdp commented Oct 22, 2015

This looks really good. Let's wait for tests to pass.

In the meantime, this will need a CHANGELOG entry, especially because it might change results for certain use cases. Actually, we might even think about having some sort of deprecation cycle for this... what do you think?

@amueller
Copy link
Member

I consider this a bugfix, so no deprecation cycle. We do need a whatsnew entry, though

@amueller amueller added this to the 0.18 milestone Dec 10, 2015
@TomDLT
Copy link
Member Author
TomDLT commented Dec 11, 2015

whatsnew entry added

@jakevdp
Copy link
Member
jakevdp commented Dec 11, 2015

From the diff, it looks like an unrelated section of whatsnew was accidentally deleted in the most recent commit.

@TomDLT
Copy link
Member Author
TomDLT commented Dec 12, 2015

This entry was duplicated

@tguillemot
Copy link
Contributor

LGTM.
@amueller @jakevdp @raghavrv Another review ?
@TomDLT Could you rebase ?

@TomDLT TomDLT changed the title [MRG] convert to boolean arrays for boolean distances (ex: jaccard) [MRG+1] convert to boolean arrays for boolean distances (ex: jaccard) May 30, 2016
raghavrv
raghavrv reviewed Jun 16, 2016
View reviewed changes
sklearn/neighbors/tests/test_neighbors.py
NN = neighbors.NearestNeighbors

nn1 = NN(metric="jaccard", algorithm='brute').fit(X)
nn2 = NN(metric="jaccard", algorithm='ball_tree').fit(X)
Copy link
Member

Choose a reason for hiding this comment

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

+1 looks neat

@raghavrv
Copy link
Member
raghavrv commented Jun 16, 2016 •
edited
Loading

LGTM apart from the minor nitpick! Thx @TomDLT !!

@TomDLT TomDLT changed the title [MRG+1] convert to boolean arrays for boolean distances (ex: jaccard) [MRG+2] convert to boolean arrays for boolean distances (ex: jaccard) Jun 16, 2016
@tguillemot
Copy link
Contributor

@TomDLT I think the pb of ci is now corrected by #6897.
Can you check it's ok but as you don't touch the doc, I think it's OK.

@raghavrv
Copy link
Member

Yes a rebase and forcepush should trigger re-build of Circle with the fixed configuration.

@TomDLT TomDLT closed this Jun 23, 2016
@TomDLT TomDLT deleted the jaccard branch June 23, 2016 17:02
@TomDLT TomDLT mentioned this pull request Jun 23, 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
0.18
Development

Successfully merging this pull request may close these issues.
6 participants
@TomDLT @jakevdp @amueller @tguillemot @raghavrv @giorgiop
0