-
Notifications
You must be signed in to change notification settings - Fork 93
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Implement is_articulation
to check if vertex is articulation point
#387
base: master
Are you sure you want to change the base?
Conversation
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 ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #387 +/- ##
==========================================
- Coverage 97.31% 97.30% -0.02%
==========================================
Files 120 120
Lines 6954 6965 +11
==========================================
+ Hits 6767 6777 +10
- Misses 187 188 +1 ☔ View full report in Codecov by Sentry. |
Bump. |
Bump :) |
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.
Thanks for the contribution, you were right to ping again
@@ -1,6 +1,6 @@ | |||
name = "Graphs" | |||
uuid = "86223c79-3864-5bf0-83f7-82e725a168b6" | |||
version = "1.11.1" | |||
version = "1.11.2" |
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you explain this shortcut in a comment?
Co-authored-by: Guillaume Dalle <[email protected]>
This factors out the DFS step from
articulation
into a separate functionarticulation_dfs!
and then uses this new function to implementis_articulation(g, v)
which checks whetherv
is an articulation point ofg
without 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 thatg
had to be connected; it does not; and 2. it changed nomenclature from "articulation point" to "cut vertex" mid-sentence).