Implement Havel-Hakimi and Kleitman-Wang graph realization algorithms #202
Conversation
Co-Authored-By: Pietro Monticone <38562595+pitmonticone@users.noreply.github.com> Co-Authored-By: Claudio Moroni <43729990+ClaudMor@users.noreply.github.com>
Codecov Report
@@ Coverage Diff @@
## master #202 +/- ##
==========================================
+ Coverage 97.43% 97.45% +0.01%
==========================================
Files 113 113
Lines 6554 6596 +42
==========================================
+ Hits 6386 6428 +42
Misses 168 168 |
|
Somehow github send me an email, that I am mentioned in this PR - but I can't find that comment anymore. It it important that this gets merged very soon? Then I can focus more on this PR than others. |
|
Hi @simonschoelly , We're sorry for the inconvenience. We have posted a this comment here by mistake: we intended to post it in #186 (where it is right now). So we'd need #186 to be merged so the master branch of our fork is free and we may move on to the next contribution. |
| """ | ||
| lexicographical_order_ntuple(A::NTuple{N,T}, B::NTuple{M,T}) where {N,T} | ||
|
|
||
| The less than (lt) function that implements lexicographical order for `NTuple`s of equal length. | ||
|
|
||
| See [Wikipedia](https://en.wikipedia.org/wiki/Lexicographic_order). | ||
| """ | ||
| function lexicographical_order_ntuple(A::Tuple{Vararg{<:Real}}, B::Tuple{Vararg{<:Real}}) | ||
| length(A) == length(B) || | ||
| throw(ArgumentError("The length of A must match the length of B")) | ||
| for (a, b) in zip(A, B) | ||
| if a != b | ||
| return a < b | ||
| end | ||
| end | ||
|
|
||
| return false | ||
| end |
There was a problem hiding this comment.
Do we need this function? Julia already implements lexicographical orders for tuples (but it also does that for tuples of different lengths. E.g.
julia> (1, 2) < (3, 4)
true
julia> (1, 2) < (1, 2)
false
julia> (1, 2) < (1, 2, 1)
true
There was a problem hiding this comment.
Right, we didn't think this functionality was implemented out of the box. We then removed lexicographical_order_ntuple.
| all(degree_sequence .>= 0) || | ||
| throw(ArgumentError("The degree sequence must contain non-negative integers only.")) |
There was a problem hiding this comment.
Would it make sense, to just call isgraphical here? Or do you think that the overhead is too big then?
There was a problem hiding this comment.
Maybe, in order to maximize performance and code simplicity, we should completely separate the functionalities of isgraphical and havel_hakimi_graph. So if the user knows that the sequence is graphical and just wants the graph, she/he may use havel_hakimi_graph, while if just/also a check for graphicality is needed, then a call to isgraphical must be made.
Then I'd suggest to either:
- Remove all checks from
havel_hakimi_graph; - Add a bool kwarg
check_graphicalitytohavel_hakimi_graph, that performs theisgraphicalcheck before executing the algorithm (in which case we may skip the subsequent check in the loop).
Similar reasoning would apply to isdigraphical and kleitman_wang_graph.
| all(collect(values(vertices_degrees_dict))[1:max_degree] .> 0) || | ||
| throw(ErrorException("The degree sequence is not graphical.")) |
There was a problem hiding this comment.
It would probably more efficient to do this check, when set vertices_degrees_dict[vertex] -= 1.
There was a problem hiding this comment.
I inserted this check in the loop. But it may be temporary, depending on what comes out of this comment.
| vertices_degrees_dict = OrderedDict( | ||
| sort(collect(vertices_degrees_dict); by=last, rev=true) | ||
| ) |
There was a problem hiding this comment.
| vertices_degrees_dict = OrderedDict( | |
| sort(collect(vertices_degrees_dict); by=last, rev=true) | |
| ) | |
| sort!(vertices_degrees_dict, byvalues=true, rev=true) |
There was a problem hiding this comment.
It throws an error https://github.com/JuliaGraphs/Graphs.jl/actions/runs/3919202599/jobs/6700055795.
So we reinstated the allocating line.
| @@ -989,6 +989,144 @@ function uniform_tree(n::Integer; rng::Union{Nothing,AbstractRNG}=nothing) | |||
| return prufer_decode(random_code) | |||
| end | |||
|
|
|||
| """ | |||
There was a problem hiding this comment.
I wonder if randgraphs.jl is the correct place for these functions, maybe staticgraphs.jl is a better place, as generators are not random.
There was a problem hiding this comment.
We moved it to staticgraphs.jl. Thanks for the suggestion.
| # Instantiate an empty simple graph | ||
| graph = SimpleGraph(length(degree_sequence)) | ||
| # Create a (vertex, degree) ordered dictionary | ||
| vertices_degrees_dict = OrderedDict( |
There was a problem hiding this comment.
As long as it work correctly, we can keep it that way, but we probably could make this function much faster, by using a Vector of tuples instead. The sorting step should also be fairly easy to speed up, as after adding a vertex, the degree order is only slightly not sorted.
There was a problem hiding this comment.
We unfortunately don't have much time for this, but it would surely help. Would even be better if the final instantiation of a graph object was optional or put in a wrapper method, so that also other packages in the ecosystem may benefit from the full performance of the method (e.g. MutlilayerGraphs.jl's configuration model-like constructors).
Co-authored-by: Simon Schölly <sischoel@gmail.com>
Co-authored-by: Simon Schölly <sischoel@gmail.com>
Co-authored-by: Simon Schölly <sischoel@gmail.com>
Co-authored-by: Simon Schölly <sischoel@gmail.com>
Co-authored-by: Simon Schölly <sischoel@gmail.com>
Co-Authored-By: Pietro Monticone <38562595+pitmonticone@users.noreply.github.com> Co-Authored-By: Claudio Moroni <43729990+ClaudMor@users.noreply.github.com>
Co-Authored-By: Pietro Monticone <38562595+pitmonticone@users.noreply.github.com> Co-Authored-By: Claudio Moroni <43729990+ClaudMor@users.noreply.github.com>
Co-Authored-By: Pietro Monticone <38562595+pitmonticone@users.noreply.github.com> Co-Authored-By: Claudio Moroni <43729990+ClaudMor@users.noreply.github.com>
@pitmonticone @ClaudMor