Conversation
aurorarossi
left a comment
aurorarossi
left a comment
There was a problem hiding this comment.
Hello! I'm not a maintainer, I'm just contributing. I wrote some suggestions and comments.
src/operators.jl
Outdated
| """ | ||
| @traitfn function mycielski(g::AbstractGraph::(!IsDirected); iterations = 1) | ||
| ref = g | ||
| out = deepcopy(g) |
There was a problem hiding this comment.
I think that deepcopy(g) could slow down and uses a lot of memory especially for big graphs.
There was a problem hiding this comment.
These operators are not supposed to mutate the input right? I see some have ! variants, but not all. Perhaps there should be a mycielski! and mycielski(g; iterations) = mycielski!(deepcopy(g); iterations=iterations) method. WDYT?
There was a problem hiding this comment.
Yes I agree with you. I was only thinking about the case in which we can mutate the input.
There was a problem hiding this comment.
I would also support having both mycielski! and mycielski. We currently have something similar for transitiveclosure and transitiveclosure! at the moment.
|
@aurorarossi thanks for the comments! |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #177 +/- ##
=======================================
Coverage 97.28% 97.29%
=======================================
Files 123 123
Lines 7406 7427 +21
=======================================
+ Hits 7205 7226 +21
Misses 201 201 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
src/operators.jl
Outdated
| Edge 10 => 11 | ||
| ``` | ||
| """ | ||
| @traitfn function mycielski(g::AbstractGraph::(!IsDirected); iterations = 1) |
There was a problem hiding this comment.
| @traitfn function mycielski(g::AbstractGraph::(!IsDirected); iterations = 1) | |
| @traitfn function mycielski(g::SimpleGraph; iterations::Int = 1) |
The problem we have at the moment, that for graphs where we have some kind of edge weights/data, it is not clear what we mean by adding an edge. This is definitely something we should sort out in the future, but in the meantime I would suggest to restrict this function to SimpleGraphs.
Also, would suggest to restrict the type of iterations to to Int or Integer.
src/operators.jl
Outdated
| """ | ||
| @traitfn function mycielski(g::AbstractGraph::(!IsDirected); iterations = 1) | ||
| out = deepcopy(g) | ||
| for _ in 1:iterations |
There was a problem hiding this comment.
| for _ in 1:iterations | |
| for _ in Base.OneTo(iterations) |
To be honest though, I am not sure if that makes a big difference for the case where iterations is not of type Int.
src/operators.jl
Outdated
| mycielski(g) | ||
|
|
||
| Returns a graph after applying the Mycielski operator to the input. The Mycielski operator | ||
| returns a new graph with `2n+1` vertices and `3e+n` edges and will increase the chromatic |
There was a problem hiding this comment.
I think using the symbol e for the number of edges is a bit unusual, when we use n for the number of vertices at the same time. Maybe there is a better way?
src/operators.jl
Outdated
| apply the operator to the previous iterations graph. | ||
|
|
||
| For each vertex in the original graph, that vertex and a copy of it are added to the new graph. | ||
| Then, for each edge `(x, y)` in the original graph, the edges `(x, y)`, `(x', y)`, and `(x, y')` |
There was a problem hiding this comment.
I think, the usual convention for variables that represent vertices are the symbols u, v and w or s and t if we talk about some path finding algorithm.
src/operators.jl
Outdated
| julia> c = CycleGraph(5) | ||
| {5, 5} undirected simple Int64 graph | ||
|
|
||
| julia> m = Graphs.mycielski(c) |
There was a problem hiding this comment.
| julia> m = Graphs.mycielski(c) | |
| julia> gm = Graphs.mycielski(g) |
We usually use m to represent some kind of number such as the number of vertices or the number of edges and use symbols such as g and h for graphs.
src/operators.jl
Outdated
| out = deepcopy(g) | ||
| for _ in 1:iterations | ||
| N = nv(out) | ||
| add_vertices!(out, N + 1) |
There was a problem hiding this comment.
This might fail, if the graph eltype does not support adding that many vertices. This might be especially bad, if we also create a mutating mycielski! function, as then this function might fail and leave g in some in-between state. So we probably should check at the start of the function if we have enough capacity to add that many vertices.
For the non-mutating mycielski function, we might also allow one to provide another larger eltype as an optional argument, such that we can retu
src/operators.jl
Outdated
| N = nv(out) | ||
| add_vertices!(out, N + 1) | ||
| w = nv(out) | ||
| for e in collect(edges(out)) |
There was a problem hiding this comment.
Here we allocate an new vector for all edges, this would need quite a lot of space. Of course it is not possible to just iterate over the edges without collecting them, but maybe we can iterate of the first N vertices and allocate just a vector of the outneighbors of each vertex?
| # ensure it is not done in-place | ||
| @test nv(g) == 15 | ||
| @test ne(g) == 50 | ||
| end |
There was a problem hiding this comment.
These tests are good, but I think we need to cover some additional test cases such as:
- tests for graphs with self-loops
- tests for graphs with zero vertices
- tests for graphs with isolated vertices
- tests for graphs with different eltypes - the
testgraphsutility function can help here
|
This is an interesting operator, I added a few suggestions. |
|
This has stalled and needs the original author to address the reviews. @mcognetta , if you have the time, your contribution would certainly be very much appreciated! For the moment I will mark this as a draft in order to have easier time organizing and sorting my review queue. |
Benchmark Results (Julia v1)Time benchmarks
Memory benchmarks
|
|
@Krastanov Hi, thanks for bringing this to my attention. I addressed most of the comments. I am not sure of the right way to handle the potential case where we use the inplace modification version but don't have enough memory to store the result. I'm happy to update this if there is a good way to handle it, but for now it will just crash. I updated the tests, the doc string, signature, etc. and I also split it into a I also implemented this function in networkx and there was a similar problem, which is why we just dumped the entire vertex set into a list at the start of each iteration, so we could rely on that for the vertex numbering and not the implicit index numbers. |
I think the problem is not with memory (as in physical memory on your computer) but with the type of the node ids in the given graph. If you have a @assert 2^iterations * (nv(g)+1) - 1 <= typemax(eltype(g)) "eltype too small (or a better error message)" |
| Edge 10 => 11 | ||
| ``` | ||
| """ | ||
| function mycielski(g; iterations=1) |
There was a problem hiding this comment.
| function mycielski(g; iterations=1) | |
| function mycielski(g::SimpleGraph; iterations=1) |
| ``` | ||
| """ | ||
| function mycielski(g; iterations=1) | ||
| has_self_loops(g) && return g |
There was a problem hiding this comment.
Is "doing nothing" the right thing for the mycielski operator on graphs with self loops? Or is the operator not defined on graphs with self loops?
| has_self_loops(g) && return g | |
| @assert !has_self_loops(g) "The graph can not have self loops" |
| @test nv(m) == 5 | ||
| @test ne(m) == 2 | ||
| end | ||
|
|
There was a problem hiding this comment.
Simple test for insufficient eltype could look like:
@testset "insufficient eltype" begin
g = complete_graph(UInt8(2))
@test_throws AssertionError mycielski!(g, iterations=10)
end| ``` | ||
| """ | ||
| function mycielski(g; iterations=1) | ||
| has_self_loops(g) && return g |
There was a problem hiding this comment.
@assert 2^iterations * (nv(g)+1) - 1 < typemax(eltype(g)) "eltype too small (or a good error message)"6 participants
This adds the
Mycielskioperator (wiki), a unary graph operator that increases the chromatic number of a graph by 1.This can be found in other graph libraries: NetworkX, SageMath.
I wasn't sure if it was best to put it here or in the generators module (which is how it is implemented in NetworkX/SageMath). Happy to move it if necessary.