-
Notifications
You must be signed in to change notification settings - Fork 140
eigen_solvers header #1421
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
eigen_solvers header #1421
Conversation
|
If cuGraph is using spectral clustering, I'd like to see us move towards a unified spectral clustering API instead of exposing this solvers API just for that library. Solvers belong in RAFT. end-to-end algorithms (like spectral clustering and spectral embedding) belong in cuVS. |
|
Agreed. This PR is just to unblock rapidsai/raft#2813 |
|
Yes, but we aren't exposing APIs in cuVS just to unblock others. We need to do it properly. APIs become much more expensive in cuVS because they are strictly compiled into cuVS (unlike raft, where they are header only). |
|
@cjnolet do you think we should discuss it with the cuGraph team for other alternatives? @ChuckHastings |
|
@jnke2016 @aamijar cuvs is not going to expose an eigen solver. That's what raft is for. If cugraph needs an eigen solver it should use raft for that. If it needs spectral clustering, it should use cuVS. If it's because we lack a spectral clustering api in cuVS which is flexible enough, we need to fix that in cuVS. No band aids. |
|
@cjnolet got it. I was asking if we should consider other alternatives like having cuGraph implementing its own spectral clustering with its primitives at some point. |
|
Thanks for the input. Yes, we are working on a proper spectral clustering API in cuvs. In the meantime are there any alternatives that cugraph can take so that we are able get this in rapidsai/raft#2813? |
|
@aamijar getting these deprecated algos removed from raft is a p0 (more important than scaling the solver). I'd like for us to solve this problem correctly instead of just trying to get something done quickly. Ideally if cugraph is using spectral clustering, we would be providing a first-class api that meets their needs. It sounds like they need spectral clustering and not just the solver. Cc @dantegd |
|
Okay, I can reprioritize to work on the Spectral Clustering first, instead of the spectral scale up. @jnke2016 Can you confirm that cugraph will be able to use a Spectral Clustering API that looks like this Is there anything else we need to consider that cugraph is blocked on? I am seeing that this is likely the file that would need to be changed in cugraph. |
@aamijar yes I can confirm. As you mentioned, the only thing cuGraph needs are the cluster/output labels
Correct, once cuVS has a version of |
|
Thank you @jnke2016 for confirming! |
|
@cjnolet Yes those solvers are not going to be exposed as you mentioned. And the new API is much simpler for cuGraph as it hides all the implementation details (solvers, kmeans , etc) |
|
Closing in favor of #1425 |
Cugraph can't include the current
.cuhimplementation, so we need to add a header.hpp.@jnke2016