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

Fix Laplacian calculation in spectral partitioning #2568

Draft
wants to merge 45 commits into
base: branch-25.04
Choose a base branch
from

Conversation

wphicks
Copy link
Contributor

@wphicks wphicks commented Feb 3, 2025

These changes enable the use of the new Lanczos solver for spectral partitioning. This solver gives the correct eigenvalues in some cases where the old solver did not, but a previous attempt to introduce this led to a downstream breakage because the Laplacian was not being calculated correctly in the spectral partitioning logic.

In this PR, we introduce a new compute_graph_laplacian function to correctly generate a new CSR matrix representing the graph Laplacian of the input CSR-formatted adjacency matrix. This resolves #2419. This function is used in the spectral partition function to obtain the correct partitioning using the new Lanczos solver. This eliminates all internal uses of computeSmallestEigenvectors, the old Lanczos implementation, which should allow it to be removed after a deprecation cycle for downstream consumers. This will in turn allow us to resolve #313.

Note that the originally-reported breakage from the previous attempt to switch to the new Lanczos solver was used to create the test confirming correct functionality of the current PR.

This PR is marked as a bugfix because it unblocks usage of the corrected Lanczos implementation. It is based on and requires PR #2541, and it will be marked as draft until that PR is merged.

@wphicks wphicks added bug Something isn't working non-breaking Non-breaking change labels Feb 3, 2025
Copy link

copy-pr-bot bot commented Feb 3, 2025

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@wphicks
Copy link
Contributor Author

wphicks commented Feb 3, 2025

The additional "vetting" is required due to missing signatures on commits in #2541. Once it's merged, I'll fix up this PR to include only the additional changes on top of the final version of #2541.

@wphicks
Copy link
Contributor Author

wphicks commented Feb 4, 2025

Also partially answers #379, though we may want additional testing beyond what is provided here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working cpp non-breaking Non-breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[FEA] Laplacian Sparse primitive for spectral algorithms [BUG] Accuracy of lanczos solver.
5 participants