-
Notifications
You must be signed in to change notification settings - Fork 525
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
Restrict HDBSCAN metric options to L2 #5415 #5492
Conversation
Pull requests from external contributors require approval from a |
/ok to test |
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.
Thank you very much for the contribution! Can you add a trivial unit test that explicitly tests this behavior to the test_hdbscan.py
module? The tests should cover the supported arguments for metric
and at least one that is not supported. In this way we can ensure consistency in the future.
@csadorf is it okay to write two unit tests one with pass condition and one with fail cases or same function with xfail mark from pytest? |
The |
/ok to test |
@Rvch7 Do you think you will have time to address my comments as well as the linter issues? Would love to see this included in the next release. Please see our CONTRIBUTING guide on how to use pre-commit hooks to avoid linter issues. |
@csadorf I will work on it tonight but I have some thinking to do with what you said about unit test as I am new to that. would u mind giving an example(pseudo code) of
if not I will figure it out eventually. |
Sure: @pytest.mark.parametrize("param,expected_to_fail", [("abc", True), ("xyz, False)])
def test(param, expected_to_fail):
if expected_to_fail:
with pytest.raises(Error):
some_func(param)
else:
some_func(param) Also have a look at this test that demonstrates how to use |
Thank you for your help @csadorf, sometimes easy things look hard at the beginning. Let me know about these changes, and also please assign me any issue you may find appropriate for me. |
/ok to test |
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.
Thank you for addressing my change requests. Just a few more minor requests and then this should be ready to merge.
/ok to test |
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!
/ok to test |
/ok to test |
/merge |
Rerunning CI now that #5536 is merged. |
/ok to test |
This PR restricts HDBSCAN metric option to only L2. if any other metric is used it gives error message "'metric' [given metric] not supported(only 'l2' or 'euclidean' is supported)"