[MRG] Fix regression in silhouette_score for clusters of size 1#6089
[MRG] Fix regression in silhouette_score for clusters of size 1#6089Sentient07 wants to merge 1 commit intoscikit-learn:masterfrom
Conversation
There was a problem hiding this comment.
I think you could also revert that deleted comment Addressed
|
This looks good to me (apart from the minor nitpick)! Wait for reviews from @MechCoder or @amueller (PS: Could you also squash the two commits? ;) ) |
There was a problem hiding this comment.
This test is not a regression test. It will pass in master.
Can you remove this and add a new test testing something like this
distances = np.random.RandomState(0).rand(5)
A = _intra_cluster_distance(distance, np.array([0, 1, 1, 1, 1]), 0)
assert_true(np.isnan(A))
|
Would be great if you could hold on till we clarify that the issue is actually an issue. |
|
it is according to the definition on wikipedia, right? (at least when I last thought about that ;) |
|
There's a merge conflict, which I realized just now. Should i resolve it or wait and do it when it's ready to be merged ? @MechCoder @amueller @rvraghav93 |
|
Resolving merge conflicts as soon as you encounter one saves a lot of headache later on, (say if you happen to accumulate more merge conflicts, it gets harder to fix them). |
|
@rvraghav93 there are some changes that has been made in the master, which i don't understand why it's been made. Like I'd go ahead with those changes ? |
8b212b9 to
0d31aaf
Compare
|
You should follow the changes in master. |
209e6d9 to
508516d
Compare
|
@rvraghav93 sorry for the delay,fixed merge conflicts. Shall i amend my commits ? |
508516d to
38cdf05
Compare
reverted the comment Resolved merge conflicts
38cdf05 to
498ccce
Compare
|
Thinking about it again, I would suggest we just raise an error for this case |
|
I have justified at f0f174b#commitcomment-18600086 why I think setting a sample score to 0 for a sample clustered alone is reasonable. I do not think an error is appropriate. |
|
fixed in #7438 |
fix #5988