-
-
Notifications
You must be signed in to change notification settings - Fork 25.8k
TST use global_dtype in sklearn/cluster/tests/test_affinity_propagation.py #22667
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
TST use global_dtype in sklearn/cluster/tests/test_affinity_propagation.py #22667
Conversation
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.
Similar remark as for #22672 for checking the dtype of the fitted attribute affinity_matrix_
and cluster_centers_
which should be float32 with fitting on float32 input data.
test_affinity_propagation.py
to test implementations on 32bit datasets.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 _equal_similarities_and_preferences
should be updated to use assert_allclose
because it compares float arrays.
Co-authored-by: Jérémie du Boisberranger <jeremiedbb@users.noreply.github.com>
I don't think it should, because this is not a testing fixture but a utility function for affinity propagation that must not raise an |
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
Right. It should use np.allclose. But it would require to create a custom allclose with different rtols. Let's leave that for later |
@@ -197,17 +205,17 @@ def test_affinity_propagation_non_convergence_regressiontest(): | |||
assert_array_equal(np.array([0, 0, 0]), af.labels_) | |||
|
|||
|
|||
def test_equal_similarities_and_preferences(): | |||
def test_equal_similarities_and_preferences(global_dtype): |
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.
The previous test (test_affinity_propagation_non_convergence_regressiontest
) should raise the warning for the different dtypes I think?
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.
and test_affinity_propagation_poredict_non_convergence
as well.
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.
The previous test (test_affinity_propagation_non_convergence_regressiontest) should raise the warning for the different dtypes I think?
Which difference of dtypes are you referring to?
Co-authored-by: Guillaume Lemaitre <g.lemaitre58@gmail.com>
I think this has been implemented in the meantime, hence I've merged |
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. I just added the global_dtype
to the 2 additional tests.
I am going to merge when it is green.
…on.py (scikit-learn#22667) Co-authored-by: Jérémie du Boisberranger <jeremiedbb@users.noreply.github.com> Co-authored-by: Guillaume Lemaitre <g.lemaitre58@gmail.com>
Reference Issues/PRs
Partially addresses #22881
Precedes #22590
What does this implement/fix? Explain your changes.
This parametrizes tests from
test_affinity_propagation.py
to run on 32bit datasets.Any other comments?
We could introduce a mechanism to be able to able to remove tests' execution on 32bit datasets if this takes too much time to complete.