MNT use scipy.sparse.csgraph.shortest_path in Isomap
#20531
Conversation
|
I assume that the remaining error is raised because we don't make any |
Co-authored-by: Guillaume Lemaitre <g.lemaitre58@gmail.com>
|
Thanks for the reviews. Now, instead of copying code from agglomerative clustering into isomap, both estimators use the same new private function |
MNT use `scipy.sparse.csgraph.shortest_path` in Isomap (scikit-learn#20531)
…20531) Co-authored-by: Guillaume Lemaitre <g.lemaitre58@gmail.com>
|
Is there a particular reason why connecting connected components is prohibited when |
|
With |
|
In the case where |
|
Thanks for insisting, you are right. There are two possibilities with
|
Takes over #14557
Fixes #14010 #8352
Closes #14557
From original pull-request:
As mentioned in the discussion in #14557, this pull-request introduces an issue with Isomap, since our own version of the graph shortest path would (wrongly) change infinite values into zeros. Infinite values can happen when there is more than one connected component in the neighbors graph. Changing to scipy's function does not change infinite values into zeros, so we could add this step back after calling scipy, but this step is wrong in the first place.
Here, I propose to raises an error instead. The question about how to best add magic to Isomap to avoid raising an error is a separate issue that should not block this PR.edit: I actually added some magic to connect disconnected components on the neighbors graph, to avoid raising errors.