-
Notifications
You must be signed in to change notification settings - Fork 182
Added a function rem_vertices! #1047
Added a function rem_vertices! #1047
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1047 +/- ##
==========================================
+ Coverage 99.81% 99.82% +<.01%
==========================================
Files 86 86
Lines 2746 2821 +75
==========================================
+ Hits 2741 2816 +75
Misses 5 5 |
Thanks. From a quick read, this looks like it's O(V+E+something). Is my estimate correct? |
Yeah that seems about right. There is a note in the comments on how it would be possible to speed it up a bit more, but from running the code it seems to be quite faster than I expected anyway. At the moment it seems that even for removing a single vertex it might be faster than |
This is surprising to me. What size graph are you trying this on? |
So either I had a very special kind of graph, where that was indeed the case, or I must have counted the number of digits wrong, because I cannot reproduce my claims at all. Looks more as if
|
Those benchmarks look more correct. Swap-n-pop is inherently efficient as it requires one move, and an O(1) resize. Does |
No it does not. The reason is, that in any case, the algorithm will check every edge. And if the order is kept, then this is also necessary most of the time.
|
Those benchmarks look suspicious. Why should removing 10k vertices take 1/8 the time that removing 5k vertices takes, and why should removing 5k vertices take half the time that removing 1k takes? |
The algorithm works in 3 phases:
So if we remove a lot of vertices, then in the third phase, there will be less lists that we have to process. For the 10k case, there will be no lists left at all. |
Thanks for the explanation. That makes sense. |
src/SimpleGraphs/simpledigraph.jl
Outdated
function rem_vertices!(g::SimpleDiGraph{T}, | ||
vs::AbstractVector{T}; | ||
keep_order::Bool=false | ||
) where {T} |
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.
If you're parameterizing SimpleDiGraph
, then you should constrain T <: Integer
. But more importantly, do you really want to insist that the vertex list is the same type as the graph eltype? Generally we just use Integer
and cast internally as appropriate.
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.
Isn't SimpleDiGraph{T}
already restricted to T <: Integer
by the definition of that datatype? I will change the vector.
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.
It is, but our convention (to date) is to constrain parameters for clarity. I'm not opposed to changing that convention if it makes sense; this was just a suggestion.
src/SimpleGraphs/simpledigraph.jl
Outdated
# Sort and filter the vertices that we want to remove | ||
remove = sort(vs) | ||
unique!(remove) | ||
lo, hi = extrema(remove) |
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.
Since remove
is sorted (I think unique
preserves the ordering), then lo, hi = (remove[1], remove[end])
is much more efficient than extrema
:
julia> a = rand(Int, 100_000_000);
julia> sort!(a);
julia> @benchmark extrema($a)
BenchmarkTools.Trial:
memory estimate: 0 bytes
allocs estimate: 0
--------------
minimum time: 53.086 ms (0.00% GC)
median time: 58.190 ms (0.00% GC)
mean time: 58.347 ms (0.00% GC)
maximum time: 65.753 ms (0.00% GC)
--------------
samples: 86
evals/sample: 1
julia> @benchmark ($a[1], $a[end])
BenchmarkTools.Trial:
memory estimate: 0 bytes
allocs estimate: 0
--------------
minimum time: 2.088 ns (0.00% GC)
median time: 2.096 ns (0.00% GC)
mean time: 2.200 ns (0.00% GC)
maximum time: 34.643 ns (0.00% GC)
--------------
samples: 10000
evals/sample: 1000
src/SimpleGraphs/simpledigraph.jl
Outdated
remove = sort(vs) | ||
unique!(remove) | ||
lo, hi = extrema(remove) | ||
(one(T) <= lo && hi <= n) || |
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.
1 <= lo <= hi <= n
works also.
src/SimpleGraphs/simpledigraph.jl
Outdated
if keep_order | ||
# traverse the vertex list and shift if a vertex gets removed | ||
i = 1 | ||
Δ = 0 |
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.
it seems like Δ
is always one behind i
. Why define it at all?
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.
Right, I don't need that.
…into remove_vertices!
See #1043
This adds a function
rem_vertices!(g, vertex_list; keep_order=false) -> vmap
. This function allows to remove multiple vertices from a graph. It returns a vectorvmap
that maps the vertices from the modified graph to the vertices in the unmodified graph. A flagkeep_order
can be set to true to ensure that the order of the vertices is not changed.