Implement is_articulation to check if vertex is articulation point#387
Implement is_articulation to check if vertex is articulation point#387Krastanov merged 10 commits intoJuliaGraphs:masterfrom
is_articulation to check if vertex is articulation point#387Conversation
|
Timing-wise, julia> g = path_graph(5)
julia> articulation(g) # [2, 3, 4]
julia> @btime 2 ∈ articulation($g) # 209.656 ns (8 allocations: 704 bytes)
julia> @btime is_articulation($g, 2) # 122.850 ns (4 allocations: 464 bytes)
julia> g = path_graph(15)
julia> articulation(g) # [2:14...]
julia> @btime 9 ∈ articulation($g) # 453.853 ns (10 allocations: 2.16 KiB)
julia> @btime is_articulation($g, 9) # 245.286 ns (4 allocations: 624 bytes) |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #387 +/- ##
==========================================
- Coverage 97.29% 97.28% -0.01%
==========================================
Files 126 126
Lines 7662 7674 +12
==========================================
+ Hits 7455 7466 +11
- Misses 207 208 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Bump. |
|
Bump :) |
gdalle
left a comment
gdalle
left a comment
There was a problem hiding this comment.
Thanks for the contribution, you were right to ping again
Project.toml
Outdated
| name = "Graphs" | ||
| uuid = "86223c79-3864-5bf0-83f7-82e725a168b6" | ||
| version = "1.11.1" | ||
| version = "1.11.2" |
There was a problem hiding this comment.
merge default branch into this PR, the version has evolved since
| s = Vector{Tuple{T,T,T}}() | ||
| low = zeros(T, nv(g)) | ||
| pre = zeros(T, nv(g)) |
There was a problem hiding this comment.
If the idea is to call is_articulation several times in a row for different vertices, these allocations might be reusable? Maybe we should expose articulation_dfs!?
There was a problem hiding this comment.
Sure, that'd be alright with me also. The _dfs part is an implementation detail though, so maybe this should just be called is_articulation!, taking work-arrays for pre and low then? Let me know what you think and I'll change to that.
There was a problem hiding this comment.
Sounds good, but then we should at the very least explain how to initialize these arguments (and ideally what they mean)
There was a problem hiding this comment.
See also discussion in #407 (comment): maybe we should just be calling these work_buffer1 and work_buffer2 (adding some comments to explain in the code - or reassigning them to more aptly named variables) to avoid exposing unnecessary complications and implementation details to users.
| if !isnothing(is_articulation_pt) | ||
| if pre[u] != 0 | ||
| return is_articulation_pt | ||
| end | ||
| end |
There was a problem hiding this comment.
Can you explain this shortcut in a comment?
Co-authored-by: Guillaume Dalle <22795598+gdalle@users.noreply.github.com>
Benchmark Results (Julia v1)Time benchmarks
Memory benchmarks
|
|
@thchr , do you have any outstanding things on this PR that you would like to do before having it merged? I updated to current master and ran the formatter and added it to the changelog. |
|
Thanks for picking this one up as well. And no, there was no other things I wanted to do here. There was a request for a comment/explanation on some aspect of the code, but since this was effectively refactoring of an earlier implementation, I didn't know the explanation off-hand and didn't have time to look more. For my sake, this is good to go as-is. |
3 participants
This factors out the DFS step from
articulationinto a separate functionarticulation_dfs!and then uses this new function to implementis_articulation(g, v)which checks whethervis an articulation point ofgwithout necessarily computing all the articulation points.The branching on
isnothing(is_articulation_pts)inarticulation_dfs!is a bit inelegant - as is the fact thatarticulation_dfs!doesn't mutate ifis_articulation_pts === nothing, but it seemed the simplest way to avoid code duplication.Aside from these additions, I revised the doc string of
articulation(g)a bit (e.g., it 1. previously misleadingly stated thatghad to be connected; it does not; and 2. it changed nomenclature from "articulation point" to "cut vertex" mid-sentence).