FIX ‘sparse’ kwarg was not used by fowlkes_mallows_score#28981
Conversation
prabashbala
left a comment
There was a problem hiding this comment.
would it be possible to add a test for this please.
adrinjalali
left a comment
There was a problem hiding this comment.
Thanks @cynddl . Could you please add a test as well?
| prefer_skip_nested_validation=True, | ||
| ) | ||
| def fowlkes_mallows_score(labels_true, labels_pred, *, sparse=False): | ||
| def fowlkes_mallows_score(labels_true, labels_pred, *, sparse=True): |
There was a problem hiding this comment.
I wouldn't change the default here though.
|
Setting Instead of "making this keyword argument work" I think it makes sense to "remove his keyword argument". It is not used, has not been used and provides no advantages. |
|
That's true, the code actually relies on the output (c) being sparse. So we can deprecate the parameter here. |
jeremiedbb
left a comment
There was a problem hiding this comment.
I made the requested changes to deprecate the param instead.
LGTM.
Reference Issues/PRs
No issue.
What does this implement/fix? Explain your changes.
The function sklearn.metrics.fowlkes_mallows_score was defined with:
but the code never uses sparse. The parameter sparse should have been passed to contingency_matrix, but instead we currently see a few lines later:
This commit fixes the inconsistency by:
Any other comments?
The function sklearn.metrics.fowlkes_mallows_score has never been tested with sparse=False, it might be good to have a few tests for that use case too?