Skip to content
This repository has been archived by the owner on Oct 8, 2021. It is now read-only.

WIP: Use SparseMatrixCSC to store graphs #150

Closed
wants to merge 16 commits into from
Closed

Conversation

jpfairbanks
Copy link
Contributor

Making the PR as a place to anchor discussion

"""Returns the backwards adjacency list of a graph.
For each vertex the Array of `dst` for each edge eminating from that vertex."""
badj(g::SimpleGraph) = g.badjlist
badj(g::SimpleGraph, v::Int) = g.badjlist[v]

badj(g::SimpleSparseGraph, v::Int) = g.m[v,:]'.rowval
badj(g::SimpleSparseGraph) = @inbounds [badj(g,i) for i in 1:nv(g)]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we could store tril(m)' in order to get efficient access to badj(g,i) just like fadj

@sbromberger
Copy link
Owner

@jpfairbanks would you mind going through operators.jl and finding ways of optimizing now that we're storing adjacency matrices directly as sparse matrices? Accessors are fmat() and bmat() for forward and backward adjacencies, respectively. (Undirected graphs have bmat == fmat.)

@sbromberger
Copy link
Owner

On a related note: since I don't anticipate tagging this before 0.4, I've gone ahead and stripped out @compat and added julia 0.4 in REQUIRES. The tipping point was the realization late last night that 0.3 doesn't have | defined for sparse matrices (even in Compat).

Any objections to this approach? We'll make the current version (0.3.3) the last supported 0.3 version and I'll tag 0.4.0 for 0.4 (nice symmetry there).

@codecov-io
Copy link

Current coverage is 86.17%

Merging #150 into master will decrease coverage by -13.83% as of 706e161

@@            master    JuliaLang/julia#150   diff @@
======================================
  Files           24      23     -1
  Stmts         1166    1309   +143
  Branches         0       0       
  Methods          0       0       
======================================
- Hit           1166    1128    -38
  Partial          0       0       
- Missed           0     181   +181

Review entire Coverage Diff as of 706e161

Powered by Codecov. Updated on successful CI builds.

@sbromberger
Copy link
Owner

Todo:

  • parallelize dijkstra and other functions
  • code coverage to 100%
  • figure out elegant way to specify parallel or nonparallel operation.

@sbromberger sbromberger changed the title Use SparseMatrixCSC to store graphs WIP: Use SparseMatrixCSC to store graphs Sep 11, 2015
@sbromberger
Copy link
Owner

What do people think about having a LightGraphs variable called parallel that was 0..n, where n is the number of workers? It could be initialized to either 0 (forcing single-process functions) or to nworkers().

This variable could be used throughout the package to determine which method (for parallelized functions) should be used.

edges::Set{Edge}
fadjlist::Vector{Vector{Int}} # [src]: (dst, dst, dst)
badjlist::Vector{Vector{Int}} # [dst]: (src, src, src)
fm::SparseMatrixCSC{Bool, Int}
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do you need two matrices to represent a directed graph?

Copy link
Owner

Choose a reason for hiding this comment

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

Primarily because it's very efficient to look up forward and backward adjacencies this way. Consider an edge (1,2). It will have fadjlist[1] = [2, ...] and badjlist[2] = [1, ...].

Note also that we're changing the game a bit with the move to sparse matrices for version 0.4. (Check out the 'sparsemx' branch for details). Directed Graphs will have two sparse matrices (accessible via fmat and bmat); undirected only needs one.

Sorry, just saw that you're commenting on sparsemx. The reason is still for efficient forward and backward adjacencies. Technically we can just transpose whenever we need the other, but I don't know what the performance implications are for sparse matrices.

@IainNZ
Copy link
Contributor

IainNZ commented Sep 21, 2015

Have you considered converting the adjacency list representation to the sparse matrix representation on-demand only for the the operations that would benefit from it?

Using a sparse matrix as essentially an adjacency list with a contiguous backing store seems really unorthodox, and I'm not sure it makes much sense for the most common use cases. It also (slightly) wasteful, requiring a useless extra byte for each edge, and makes operations such as dynamically adding or removing nodes and edges more expensive/complicated.

@sbromberger
Copy link
Owner

@IainNZ Yes - in fact, that's what we currently do.

This effort is by no means a fait accompli - it's really just a proof of concept to see whether it makes sense to change the underlying representation of graphs. Right now there's not much arguing against making a switch, but it's still early days and roadblocks can come up. Your opinions/objections are very much welcome!

@sbromberger
Copy link
Owner

After this effort, I'm not convinced that this provides any significant benefit, and has some risks that may cause problems down the road. It seems, frankly, that sparse matrices are "second class citizens" right now in Julia, and if that ever changes we can see a slew of modifications to the class that may require lots of work here.

I'll close this out now but will be incorporating the non-sparse improvements into the current master.

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

Successfully merging this pull request may close these issues.

None yet

4 participants