Adding support for edge weights (using SimpleWeightedGraphs) #37
Conversation
|
@fredrikekre, could you please review this PR? I'd like to use the support for edge weights. |
|
Superficially LGTM but I don't know anything about the graph library so would be good to get a review from someone active there (@jpfairbanks or @matbesancon maybe? sorry if you are the wrong people to ping here.)
Edit: Ah sorry I missed that you asked about those things in the PR body
The easiest is to start Julia in the root of the package with the
At a minimum it would be good to test the code paths you add here, like constructing the graph etc. If there are no simple examples with weights, one easy check would be to check that the result of the unweighted partition matches the weighted partiton if all weights are 1 or something like that (assuming they should be equivalent). |
|
I've added SimpleWeightedGraphs to the package dependencies. I noticed that setting the vertex weights to 0 results in weird behavior in METIS (I actually tested this running the METIS library directly in a separate installation) so now the I've added a simple test for the edge weight support. Following @fredrikekre 's suggestion of comparing the partition obtained from setting the edge weights to 1 with the partition obtained with the unweighted version. |
|
LGTM even though I don't have much knowledge of Metis. For the weight of 0, SimpleWG is a sparse matrix under the hood, so adding an edge with a weight of 0 will not add the edge I believe (there are checks to |
I get what you say regarding edge weights, but I’m referring to vertex weights. I couldn’t construct a |
| N = SimpleWeightedGraphs.nv(G) | ||
| xadj = Vector{idx_t}(undef, N+1) | ||
| xadj[1] = 1 | ||
| adjncy = Vector{idx_t}(undef, 2*SimpleWeightedGraphs.ne(G)) |
There was a problem hiding this comment.
There is a discrepancy between SimpleWeightedGraphs.ne() and LightGraphs.ne() because SimpleWeightedGraphs.ne() fails to count the correct number of self-loops (please see here). The current Metis.partition() is able to handle unweighted graphs with self-loops, so I think this part should be fixed to allow self-loops.
There was a problem hiding this comment.
At least the following implementation avoids the problem.
adjncy = Vector{idx_t}(undef, 2*SimpleWeightedGraphs.ne(G)+N)| xadj[1] = 1 | ||
| adjncy = Vector{idx_t}(undef, 2*SimpleWeightedGraphs.ne(G)) | ||
| vwgt = ones(idx_t, N) | ||
| adjwgt = Vector{idx_t}(undef, 2*SimpleWeightedGraphs.ne(G)) |
|
It would be helpful if some examples for performing node and edge-weighted grid partition are added. As Iam struggling with the creation of how SparseCSC can be created for creating node and edge weighted graphs. (like to sparse CSC for the required graph is created, adding edge and node weight, performing the metis) Thanks in advance |
|
#45 adds weight support for sparse matrix input. Shouldn't be too difficult to rebase this PR after that is merged. |
This patch adds support for edge weights based on the SimpleWeightedGraphs package. This is implemented as a package extension, and thus requires Julia 1.9 or newer. Closes #37. Co-authored-by: Rodolfo Carvajal <[email protected]> Co-authored-by: Fredrik Ekre <[email protected]>
This patch adds support for edge weights based on the SimpleWeightedGraphs package. This is implemented as a package extension, and thus requires Julia 1.9 or newer. Closes #37. Co-authored-by: Rodolfo Carvajal <[email protected]> Co-authored-by: Fredrik Ekre <[email protected]>
Hello! First of all, please note that I'm quite new to Julia and this is my first time contributing to a project.
Inspired by #35, I decided to modify the
partitionfunction in order to be able to use weights on edges. At first I thought of just adding an extra argument with theadjwgtvector with the weights, but this vector would have to be constructed by the user in the Metis CSR format which is not very friendly.So, I decided to add a method for the
graph()function that converts a weighted graph from SimpleWeightedGraphs.jl into a CSR graph.A few things that I'm not sure about (and that I would be thankful to know your opinions on):
SimpleWeightedGraphsto the package's dependencies. Every time I dousing Metisin the REPL I get the message: "Warning: Package Metis does not have SimpleWeightedGraphs in its dependencies".Graphthat would ignore thevwgtfield but notadjwgt. What I ended up doing was settingvwgtto a vector of zeros (this should not affect the behavior of METIS).Thanks in advance for the chance of contributing!