Add mycielski operator
#177
Conversation
| """ | ||
| @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
@@ Coverage Diff @@
## master #177 +/- ##
=======================================
Coverage 97.47% 97.48%
=======================================
Files 109 109
Lines 6349 6366 +17
=======================================
+ Hits 6189 6206 +17
Misses 160 160 |
| 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.
| """ | ||
| @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.
| 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?
| 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.
| 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.
| 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
| 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 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.