Implement Havel-Hakimi and Kleitman-Wang graph realization algorithms#202
Implement Havel-Hakimi and Kleitman-Wang graph realization algorithms#202InterdisciplinaryPhysicsTeam wants to merge 11 commits intoJuliaGraphs:masterfrom
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❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #202 +/- ##
==========================================
+ Coverage 97.43% 97.71% +0.27%
==========================================
Files 113 122 +9
Lines 6554 9454 +2900
==========================================
+ Hits 6386 9238 +2852
- Misses 168 216 +48 🚀 New features to boost your workflow:
|
|
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.
| 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>
|
closing and reopening to rerun the tests |
| ) | ||
| # Find a vertex with positive outdegree,and temporarily remove it from `vertices_degrees_dict` | ||
| i, (a_i, b_i) = 0, (0, 0) | ||
| for (_i, (_a_i, _b_i)) in collect(deepcopy(vertices_degrees_dict)) |
There was a problem hiding this comment.
any reason this is a deepcopy and not just a normal copy?
|
@InterdisciplinaryPhysicsTeam , @pitmonticone , @ClaudMor -- any reason to not merge this? It seems you have addressed most of the review comments (there is one more deepcopy that you probably should turn into a copy) Could you rerun the formatter on this? |
|
Maintainer edits have been disabled on this PR, so I can not run the formatting myself. I will mark this is a draft to keep my review queue a bit more organized, but the only outstanding issue seems to be the formatting check. Please do not hesitate to convert it back to not a draft if you have the bandwidth to pass this last hurdle. |
6 participants
@pitmonticone @ClaudMor