MAINT Plug PairwiseDistancesArgKmin as a back-end
#22214
Conversation
PairwiseDistancesArgKmin as a back-end
sklearn/metrics/pairwise.py
Outdated
| ) | ||
| indices = indices.flatten() | ||
| else: | ||
| # TODO: once PairwiseDistancesArgKmin supports sparse input matrices and 32 bit, |
There was a problem hiding this comment.
Are there options to get 32bit to work besides `Tempita?
There was a problem hiding this comment.
Apart from duplication based on dtype in Cython, I hardly can see something else.
C++ template classes would come in handy here.
There was a problem hiding this comment.
I guess, we can start with Tempita first and see how this makes the code more complex.
What do the others think?
There was a problem hiding this comment.
If it works with fused types in Cython, that would be great. If not, C++ and Tempita a both fine for me.
There was a problem hiding this comment.
Fused types can't be used for attributes unfortunately (hence the need to use Tempita similarly to what exists for WeightVectors).
sklearn/neighbors/_base.py
Outdated
| raise ValueError("p must be greater or equal to one for minkowski metric") | ||
|
|
||
| def _fit(self, X, y=None): | ||
| if self._get_tags()["requires_y"]: | ||
| if not isinstance(X, (KDTree, BallTree, NeighborsBase)): | ||
| X, y = self._validate_data(X, y, accept_sparse="csr", multi_output=True) | ||
| X, y = self._validate_data( | ||
| X, y, accept_sparse="csr", multi_output=True, order="C" |
There was a problem hiding this comment.
Note to other reviewers: this is required by the new Cython implementation. It triggers a potential memory copy in the rare cases where X is not already contiguous (even in cases when we cannot use the new Cython code (e.g. for float32 data that are not supported yet).
But as the plan will be to also write the Cython code for the 32 bit float in the future, let's keep the input data validation logic simpler by always enforcing contiguity from the start.
|
#22241 has been opened to fix a test failure. |
dce919f to
3448b01
Compare
There was a problem hiding this comment.
Looking over the tests, a bunch of the changes look like refactors for using pytest's parametrize. I think they can be broken out into their own PRs and merged directly into main.
Implementation looks good to me. The major change is having to set order="C" everywhere, which I think that is okay.
sklearn/multioutput.py
Outdated
| @@ -399,7 +399,7 @@ class MultiOutputClassifier(ClassifierMixin, _MultiOutputEstimator): | |||
| >>> X, y = make_multilabel_classification(n_classes=3, random_state=0) | |||
| >>> clf = MultiOutputClassifier(KNeighborsClassifier()).fit(X, y) | |||
| >>> clf.predict(X[-2:]) | |||
| array([[1, 1, 0], [1, 1, 1]]) | |||
| array([[1, 1, 1], [1, 1, 1]]) | |||
There was a problem hiding this comment.
Does this mean the new implementation gives different results than the previous one?
There was a problem hiding this comment.
test_neighbors_metrics normally test consistency of the three algorithms' results.
Yet, in the specific case of this doc-test, the result is different.
I think the best is to test the new 'brute' back-end against the current implementation.
I will come up with test for this.
|
@thomasjpfan: To address your comments, some changes made in this PR have been extracted in #22280 and #22281. |
cb27fdf to
92a23ba
Compare
|
Superseded by #22288. |
Reference Issues/PRs
Part of #21462 (comment).
What does this implement/fix? Explain your changes.
This plugs
PairwiseDistancesArgKminas a back-end so that it is used for some API, namely:sklearn.metrics.pairwise_distances_argminsklearn.metrics.pairwise_distances_argmin_minsklearn.cluster.AffinityPropagationsklearn.cluster.Birchsklearn.cluster.MeanShiftsklearn.cluster.OPTICSsklearn.cluster.SpectralClusteringsklearn.feature_selection.mutual_info_regressionsklearn.neighbors.KNeighborsClassifiersklearn.neighbors.KNeighborsRegressorsklearn.neighbors.LocalOutlierFactorsklearn.neighbors.NearestNeighborssklearn.manifold.Isomapsklearn.manifold.LocallyLinearEmbeddingsklearn.manifold.TSNEsklearn.manifold.trustworthinesssklearn.semi_supervised.LabelPropagationsklearn.semi_supervised.LabelSpreadingExisting tests are adapted and completed accordingly.
Any other comments?
Currently checking if any original change has been left behind.