-
Notifications
You must be signed in to change notification settings - Fork 26
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
Add kmeans clustering function #286
Conversation
07e47aa
to
a199c60
Compare
@pytest.mark.parametrize("method", ["spearman_correlation", "auto_scale"]) | ||
@pytest.mark.parametrize( | ||
"num_polynomials", | ||
tuple(range(1, 5)) + (20, 100), | ||
) | ||
def test_misfit_preprocessor_n_polynomials(num_polynomials, method): | ||
def test_misfit_preprocessor_n_polynomials( |
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.
I think in this case, you get a cleaner implementation with hypothesis than with pytest parametrization.
Just a suggestion:
from hypothesis import given, assume
import hypothesis.strategies as st
clustering_functions = st.sampled_from(["hierarchical", "kmeans"])
methods = st.sampled_from(["spearman_correlation", "auto_scale"])
@given(st.integers(min_value=1, max_value=100), methods, clustering_functions)
def test_misfit_preprocessor_n_polynomials(
num_polynomials, method, clustering_function
):
if (
clustering_function == "kmeans"
and method == "spearman_correlation"
):
assume(num_polynomials in [4,5]) # or more
0ad4146
to
2ce2e4f
Compare
Had some minor comments, otherwise I think the implementation looks good! |
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! Nice job!
45408ed
to
2a7c235
Compare
Closes #241