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

Speed and scalability improvements for graph multiresolution #3

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

EiffL
Copy link
Contributor

@EiffL EiffL commented Jun 4, 2017

The motivation for these modification is to improve the scalability of the code to be able to handle much larger graphs.

With the baseline code, I couldn't compute graph pyramids for graphs with about 40,000 nodes, the peak memory consumption went above 500GB ! Turns out this was due to 2 main problems:

  • In the kron_reduction function, the computation of the Schur complement was actually turning back the sparse input matrices into dense matrices. Plus, the computation of the Schur complement can be sped up by using a linear equation solver optimized to handle SDD matrices (symmetric diagonally dominant), which is the case of graph laplacians. There are even more efficient algorithms out there (starting with Cholesky decomposition) such as Koutis et al. (2011) arXiv:1102.4842 but I didn't find an out of the box python implementation and SuperLU seemed to do the job for me. I ended up rewriting bits of kron_reduction to ensure that the sparse matrices were not turned into dense matrices when not necessary, and I added pygsp.utils.splu_inv_dot as an optimized alternative to the use of scipy.sparse.spsolve .

  • In the graph_sparsify function, the computation of the resistance distances was done using blunt matrix inversion in pygsp.utils.resistance_distance which tries to invert it a potentially very large Laplacian matrix. After a kron reduction, this laplacian can have a lot of non zero elements, that's what was causing the worse of the memory consumption in my case. However, it turns out that the whole point of the Spielman-Srivastava spectral sparsification algorithm is to avoid having to compute this inverse, the algorithm only requires approximate distances and provides a procedure to compute them. I implemented pygsp.utils.approx_resistance_distance to compute these distances based on the algorithm described in arxiv:0803.0929 . It still requires a way of solving a linear inverse system, where I used again my customized pygsp.utils.splu_inv_dot but it should be noted that there are much faster ways of doing this. Including the near linear time algorithm described in arXiv:1209.5821v3

With these modifications, I can now compute graph multiresolutions in practice for graphs with about 50,000 nodes and 1e6 edges without running into memory issues on a standard desktop. The slowest point in the process is the sampling time for the edges in graph_sparsify, scipy.stats.rv_discrete takes about 1s to sample 10,000 deviates, in my case the algorithm needed 16e6 which takes about 30min just to sample random numbers whereas the rest of the algorithm only takes minutes, so it's really stupid, but at least it eventually works.

I tried to comment and document these modifications in the hope that they might be useful to others, everything seems to work for me but someone should take a close look to check that it doesn't break anything.

@rodrigo-pena
Copy link

Hi, thanks for the contribution. Could you check and correct the errors that were raised in the TravisCI checks? They seem to be mostly errors in the code examples in the documentation, raising errors such as "NameError: name 'sparse' is not defined", "NameError: name 'extract_submatrix' is not defined", or "NameError: name 'block' is not defined".

@rodrigo-pena
Copy link

rodrigo-pena commented Jun 15, 2017

There's still a silly error in the doctest (it's one that I often forget is a problem):

pygsp/pygsp/utils.py", line 261, in utils.py
Expected:
(8, 8)
"""
M = M.tocoo()
Got:
(8, 8)

There should be a newline after (8,8) in the documentation

@EiffL
Copy link
Contributor Author

EiffL commented Jun 15, 2017

My bad, it should be fine now.

Copy link

@rodrigo-pena rodrigo-pena left a comment

Choose a reason for hiding this comment

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

Hi, could you check my comments on your pull request?

if self.directed or not self.connected:
raise NotImplementedError('Focusing on connected non directed graphs first.')

start_nodes, end_nodes, weights = sparse.find(sparse.tril(self.W))

Choose a reason for hiding this comment

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

Do you know if this is faster/slower/same as what's done in adj2vec(), lines 23-24 in pygsp/data_handling.py? Indeed, I rather prefer your design, than the use of adj2vec we're making now for computing graph gradients. I might make the necessary adaptions to replace it by this call of your create_incidence_matrix soon.

@@ -12,17 +12,26 @@
logger = build_logger(__name__)


def graph_sparsify(M, epsilon, maxiter=10):
def graph_sparsify(M, epsilon, maxiter=10, fast=True):

Choose a reason for hiding this comment

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

I would set fast=False by default, so that the previous default behavior of this function remains unchanged. What do you think?

@@ -295,12 +300,22 @@ def kron_reduction(G, ind):
Graph structure or weight matrix
ind : list
indices of the nodes to keep
threshold: float
Threshold applied to the reduced Laplacian matrix to remove numerical
noise. (default: marchine precision)

Choose a reason for hiding this comment

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

typo: machine

Lnew.eliminate_zeros()

# Enforces symmetric Laplacian
Lnew = (Lnew + Lnew.T) / 2.

Choose a reason for hiding this comment

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

Do we always want the Laplacian to be symmetric here? In the previous implementation, this line was called only under the conditional statement.

@mdeff
Copy link
Collaborator

mdeff commented Aug 10, 2017

Thanks for the contribution. :) Could you look at @rodrigo-pena's comments so that we can merge it ?

@coveralls
Copy link

Coverage Status

Coverage increased (+1.7%) to 81.844% when pulling bf2183e on EiffL:faster into ef8823a on epfl-lts2:master.

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

Successfully merging this pull request may close these issues.

None yet

4 participants