Skip to content
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

[FEA] Lanczos solver v2 #2481

Open
wants to merge 32 commits into
base: branch-24.12
Choose a base branch
from

Conversation

lowener
Copy link
Contributor

@lowener lowener commented Nov 1, 2024

I unfortunately don't have permissions to push on @aamijar branch for the previous Lanczos solver PR (#2416) so I kept his commits and continued it here.

Lanczos Solver for Sparse Eigen Decomposition

We propose a new lanczos solver in raft that fixes the issues present in the previous solver raft::sparse::solver::detail::computeSmallestEigenvectors.

Specifically we address the following issues:

  1. Numerical Stability for both float32 and float64 datatypes
  2. Efficiency and Speed of Convergence

This new implementation is taken from the cupy library cupyx.scipy.sparse.linalg.eigsh where the thick-restart and full reorthogonalzation methods are used.

Additionally this PR exposes a python api for raft lanczos solver with an interface similar to scipy.sparse.linalg.eigsh and cupyx.scipy.sparse.linalg.eigsh.

from pylibraft.solver import eigsh

@lowener lowener requested review from a team as code owners November 1, 2024 21:28
Copy link
Contributor

@KyleFromNVIDIA KyleFromNVIDIA left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved CMake changes

@lowener lowener added 3 - Ready for Review non-breaking Non-breaking change feature request New feature or request labels Nov 1, 2024
@lowener
Copy link
Contributor Author

lowener commented Nov 1, 2024

/ok to test

@lowener
Copy link
Contributor Author

lowener commented Nov 1, 2024

/ok to test

@lowener
Copy link
Contributor Author

lowener commented Nov 2, 2024

/ok to test

@lowener
Copy link
Contributor Author

lowener commented Nov 4, 2024

/ok to test

ValueTypeT one = 1;
ValueTypeT mone = -1;

// Using raft::linalg::gemv leads to Reason=7:CUBLAS_STATUS_INVALID_VALUE
Copy link
Member

@cjnolet cjnolet Nov 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you create an issue for follow-on tasks and add fixing this to the issue? (and reference the issue here, please). We shouldn't be using internal detail APIs across namespaces but in the interest of getting this PR merged quickly, I think we should create an issue for it and move on.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See #2484

template <typename IndexTypeT, typename ValueTypeT>
auto lanczos_compute_smallest_eigenvectors(
raft::resources const& handle,
raft::device_vector_view<IndexTypeT, uint32_t, raft::row_major> rows,
Copy link
Member

@cjnolet cjnolet Nov 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should be exposing public APIs only for our sparse types and not accepting those arrays individually. I'm okay keeping this in the meantime, but please add this to the follow-on issue and reference it here.

@lowener
Copy link
Contributor Author

lowener commented Nov 5, 2024

/ok to test

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants