ENH Add dtype preservation for SpectralClustering
#22669
Conversation
jjerphan
changed the title
test_spectral.py to test implementations on 32bit datasets
|
According to some irl discussions, such tests should only be added after Let's keep this PR open in the mean time. |
cmarmo
added
float32
|
Resurrecting this PR: |
jjerphan
changed the title
SpectralClustering
jjerphan
added
the
Waiting for Second Reviewer
| self.affinity_matrix_ = pairwise_kernels( | ||
| X, metric=self.affinity, filter_params=True, **params | ||
| ) | ||
| ).astype(X.dtype, copy=False) |
There was a problem hiding this comment.
I think we should work on making pairwise_kernels work efficiently with float32 data first.
Since affinity="rbf" is the default, flagging SpectralClustering as dtype preserving without that prerequisite would be misleading to our users: there would be very few performance or peak memory usage gains in passing float32 data to this estimator when using the default hyper-params.
There was a problem hiding this comment.
BTW we also need a test that checks the dtype of affinity_matrix_ when the affinity param is the str name of a kernel function and another such assertion in a test that covers the case when affinity is nearest_neighbors.
There was a problem hiding this comment.
We also need to have ArgKmin and RadiusNeighbors preserve dtypes because it is also used when affinity="precomputed_nearest_neighbors".
Let's turn this PR as a draft for now.
| .fit(S) | ||
| .labels_ | ||
| ) | ||
| assert adjusted_rand_score(y, labels) == 1 |
There was a problem hiding this comment.
Please check the dtype of affinity_matrix_ here.
| ) | ||
| results.append(labels) | ||
|
|
||
| assert_array_equal(results[0], results[1]) |
There was a problem hiding this comment.
Please check the dtype of affinity_matrix_ here.
glemaitre
removed
the
Waiting for Second Reviewer
|
Closing for now, might reopen later. |
5 participants
Reference Issues/PRs
Partially addresses #22881
Precedes #22590
What does this implement/fix? Explain your changes.
This parametrizes tests from
test_spectral.pyto 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.