-
Notifications
You must be signed in to change notification settings - Fork 182
improved performance of core_number - credit to @abhinavmehndiratta #1281
Conversation
sbromberger
commented
Jan 26, 2020
•
edited
Loading
edited
4ebba74
to
c907f60
Compare
Codecov Report
@@ Coverage Diff @@
## master #1281 +/- ##
=======================================
Coverage 99.66% 99.66%
=======================================
Files 104 104
Lines 5010 5010
=======================================
Hits 4993 4993
Misses 17 17 |
c907f60
to
cc937b6
Compare
@@ -1,5 +1,3 @@ | |||
# Code in this file inspired by NetworkX. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we remove this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought I had.
(edit: yes. That's what the red / -
means :) )
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sorry I meant as in, should this stay?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think so. The code was a 1:1 transcription of NetworkX before; now it's not. There's nothing tying this to NetworkX at this point.
src/degeneracy.jl
Outdated
n = nv(g) | ||
deg = T.(degree(g)) # degree should really return T. | ||
maxdeg = maximum(deg) | ||
bin = zeros(T, maxdeg+1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This makes a lot of very short names here, should we either comment on what each is and/or use some longer names for the ones used throughout the function (not the ones local to a single for loop or so
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll defer to @abhinavmehndiratta to provide better variable names.
Other than the two minor things I left, this looks good, as it's just accelerating an existing algorithm without changes in apparent behaviour, it should be fairly consensual |
@abhinavmehndiratta is there potential for parallelizing this? |
@sbromberger function core_number(g::AbstractGraph{T}) where T
has_self_loops(g) && throw(ArgumentError("graph must not have self-loops"))
n = nv(g)
deg = T.(degree(g)) # this will contain core number for each vertex of graph
maxdeg = maximum(deg) # maximum degree of a vertex in graph
bin = zeros(T, maxdeg+1) # used for bin-sort and storing starting positions of bins
vert = zeros(T, n) # contains the set of vertices, sorted by their degrees
pos = zeros(T, n) # contains positions of vertices in array vert
# count number of vertices will be in each bin
for v = 1:n
bin[deg[v]+1] += one(T)
end
# from bin sizes determine starting positions of bins in array vert
start = one(T)
for d = zero(T):maxdeg
num = bin[d+1]
bin[d+1] = start
start += num
end
# sort the vertices in increasing order of their degrees and store in array vert
for v in vertices(g)
pos[v] = bin[deg[v]+1]
vert[pos[v]] = v
bin[deg[v]+1] += one(T)
end
# recover starting positions of the bins
for d = maxdeg:-1:one(T)
bin[d+1] = bin[d]
end
bin[1] = one(T)
# cores decomposition
for i = 1:n
v = vert[i]
# for each neighbor u of vertex v with higher degree we have to decrease its degree and move it for one bin to the left
for u in all_neighbors(g, v)
if deg[u] > deg[v]
du = deg[u]
pu = pos[u]
pw = bin[du+1]
w = vert[pw]
if u != w
pos[u] = pw
vert[pu] = w
pos[w] = pu
vert[pw] = u
end
bin[du+1] += one(T)
deg[u] -= one(T)
end
end
end
return deg
end |
…s/LightGraphs.jl into sbromberger/core_number
Once this passes tests, given @matbesancon 's and my review, I'll merge. |