Adding checks for UMAP precomputed KNN feature

[viclafargue]

Closes #5717

[csadorf]

Is the expectation that the MRE provided in #5717 just fails with ValueError now?

[viclafargue]

Is the expectation that the MRE provided in #5717 just fails with ValueError now?

I just updated the PR so that if the number of neighbors provided in the precomputed KNN graph is higher than the number of requested neighbors, the graph is trimmed instead of raising an exception.

[csadorf]

Is the expectation that the MRE provided in #5717 just fails with ValueError now?

I just updated the PR so that if the number of neighbors provided in the precomputed KNN graph is higher than the number of requested neighbors, the graph is trimmed instead of raising an exception.

How does umap-learn handle this case?

[viclafargue]

How does umap-learn handle this case?

In the case of more neighbors than requested it trims the pre-computed KNN graph. In case there are less than required it displays a discrete warning, drops the pre-computed graph and proceed with the computation of a new KNN graph from scratch. But, I feel like raising an exception here is a better choice to alert the user in case the KNN graph has an insufficient number of nearest neighbors instead of almost silently computing a new graph. What do you think?

[csadorf]

How does umap-learn handle this case?

In the case of more neighbors than requested it trims the pre-computed KNN graph. In case there are less than required it displays a discrete warning, drops the pre-computed graph and proceed with the computation of a new KNN graph from scratch. But, I feel like raising an exception here is a better choice to alert the user in case the KNN graph has an insufficient number of nearest neighbors instead of almost silently computing a new graph. What do you think?

Yes, absolutely. I agree that matching the behavior w.r.t. to pruning makes sense. We should however fail with an exception instead of computing the KNN graph on an insufficient number of neighbors. We can implement a 100% match of the behavior in cuml.accel only.

[csadorf]

One comment, but overall LGTM! Thanks a lot!

[viclafargue]

/merge