-
-
Notifications
You must be signed in to change notification settings - Fork 26.5k
[MRG] Prevent division by zeros in davies_bouldin_score and improve docstring - Fixes #12611 #12760
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.
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
Conversation
Replaces 0s with infinity to make division by 0 result in 0
jnothman
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add a non-regression test that ensures no warning is raised
|
Is there any way I can can run the tests without building? I don't have C++ build tools installed, nor do I have admin access on my machine to install them. |
|
The tests are run here in any case, and pass. It should be possible to run tests on an installed package, but I don't know a neat way to install your working copy without a compiler or some hackery. |
|
You should not need admin access, though. conda for instance will happily install build tools without super-user rights. You just need to first install conda in user space. |
eamanu
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Like say jnothman is good idea use np.max alias. whatsnew?
|
I don't understand what's going on with the codecov test; if I'm understanding the output correctly, it's reflecting changes that I didn't make in these commits. |
|
I'm not sure why it's failing again. It just says |
|
Please merge the latest master. Circle CI config has changed |
|
Anything else I need to do to get this merged? |
|
Wait for a second core developer to review |
|
Pinging, as you have, helps :) |
agramfort
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
|
+1 for MRG provided the test added fails on master. |
|
jnothman
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure that this needs a what's new.
…rove docstring (scikit-learn#12760)" This reverts commit a72b4fa.
…rove docstring (scikit-learn#12760)" This reverts commit a72b4fa.
Prevent division by zeros warning in
davies_bouldin_scoreby replacing zero intracluster distances withnp.inf. This results in division with those values returning zero instead ofnp.nan, which is intuitive (clusters are identical to themselves, so their similarity to themselves is 0). This fixes #12611.Also improves the docstring for
davies_bouldin_scoreby adding a bit more detail on how it works and what value is expected.