FIX binary/multiclass jaccard_similarity_score and extend to handle averaging#13092
FIX binary/multiclass jaccard_similarity_score and extend to handle averaging#13092jnothman wants to merge 97 commits intoscikit-learn:masterfrom
Conversation
this deals with both multilabel and multiclass problems
labels, sample_weight seems to be working fine, though haven't fully testing them again, will do in next commit
|
We might also consider renaming |
|
|
||
| def jaccard_similarity_score(y_true, y_pred, normalize=True, | ||
| def jaccard_similarity_score(y_true, y_pred, labels=None, pos_label=1, | ||
| average='samples', normalize='true-if-samples', |
There was a problem hiding this comment.
just a note that this is not backward compatible with users calling it with positional arguments [sigh]! But I'm not sure what we should do in these cases.
There was a problem hiding this comment.
If we deprecate the current function and make jaccard_score that would solve it :)
| labels are column indices. By default, all labels in ``y_true`` and | ||
| ``y_pred`` are used in sorted order. | ||
|
|
||
| pos_label : str or int, 1 by default |
| ... # doctest: +ELLIPSIS | ||
| 0.33... | ||
| >>> jaccard_similarity_score(y_true, y_pred, average='micro') | ||
| ... # doctest: +ELLIPSIS |
There was a problem hiding this comment.
I think this is redundant, it's already set above (and it generates the odd empty ... line in the output).
There was a problem hiding this comment.
I think these flags are per-statement, so I don't see how "it's already set above"
There was a problem hiding this comment.
The scope of those flags are at least per-block, example:
scikit-learn/sklearn/covariance/empirical_covariance_.py
Lines 128 to 132 in 8d10ba0
| def test_jaccard_similarity_score_validation(): | ||
| y_true = np.array([0, 1, 0, 1, 1]) | ||
| y_pred = np.array([0, 1, 0, 1, 1]) | ||
| assert_raise_message(ValueError, "pos_label=2 is not a valid label: " |
There was a problem hiding this comment.
why pytest.raises? For readability? I don't think the error message is any better with pytest.raises for instance.
There was a problem hiding this comment.
aren't we gradually moving away from assert_raise_message and move to with pytest.raises(...)? At least that was my impression.
| "classification.") | ||
| assert_raise_message(ValueError, msg3, jaccard_similarity_score, y_true, | ||
| y_pred, average='samples') | ||
| assert_raise_message(ValueError, msg3, jaccard_similarity_score, y_true, |
There was a problem hiding this comment.
duplicate of the above? seems like a copy/paste issue.
|
Thanks for the review @adrinjalali! |
|
It's also probably a good idea to check if there need to be changes to |
I'll vote +1 for this solution. And I don't understand why we need |
| Calculate metrics for each instance, and find their average (only | ||
| meaningful for multilabel classification). | ||
|
|
||
| normalize : bool, optional (default=True) |
There was a problem hiding this comment.
default is true-if-samples and not True
|
well, I guess #13151 is a better solution anyway :) |
|
I think there's enough consensus to close this one. Let's try to merge #13151 |
Reference Issues/PRs
Fixes #7332. Supersedes #10083
What does this implement/fix? Explain your changes.
The current Jaccard implementation is ridiculous for binary and multiclass problems, returning accuracy. This makes Jaccard comparable to Precision, Recall and F-score, which are also fundamentally set-wise metrics.