-
Notifications
You must be signed in to change notification settings - Fork 126
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
added graph_from_edges function #3300
Conversation
How can I create a graph with isolated vertices using this function? Maybe one can use something like permutation constructors from cycles where there are fixpoints in the permutation? |
I have added an optional argument called additional_vertices, which you can use to create isolated vertices. |
@@ -1081,3 +1081,63 @@ function Base.show(io::IO, G::Graph{T}) where {T <: Union{Polymake.Directed, Po | |||
print(io, "$(_to_string(T)) graph with $(nvertices(G)) nodes and $(nedges(G)) edges") | |||
end | |||
end | |||
|
|||
function graph_from_edges(::Type{T}, edges::Vector{Edge}, additional_vertices::Vector{Int}=Int[]) where {T <: Union{Directed, Undirected}} |
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 think it makes more sense to just allow an optional n_vertices
:
function graph_from_edges(::Type{T}, edges::Vector{Edge}, additional_vertices::Vector{Int}=Int[]) where {T <: Union{Directed, Undirected}} | |
function graph_from_edges(::Type{T}, edges::Vector{Edge}; n_vertices::Int=-1) where {T <: Union{Directed, Undirected}} |
and then later something like
n = maximum(verts)
if n_vertices>-1
@req n_vertices >= n "Edges require more vertices than allowed."
n = maximum(n, n_vertices)
end
My reasons are the following:
- If you call
graph_from_edges([[1,3]])
it will have an isolated vertex at2
anyway. (You do this vertex removal later, but why would you want that? It seems intransparent to the user, maybe make it a separate thing. polymake hassqueeze
for that.) - Graphs always have vertices 1...n.
- You never check whether
add_vertex!
returns false. You should probably also check this foradd_edge
anyway.
Maybe also set a default for 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.
I disagree, the change to an optional argument n_vertices
once again puts the effort on the user to calculate the total amount his edges plus desired isolated vertices require. This completely defeats the purpose of a highly requested, easy to use function to create a graph. I know it is a trivial thing to do, but still work to be done.
Similarly, it feels much more counterintuitive that graph_from_edges([[1,3]])
will give you an unwanted isolated vertex, rather than just a graph with one edge and two vertices. I will think about a way to make vertex removal transparent for the user.
I will check out squeeze
from Polymake and add a default type for T
. Thanks a lot :)
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## master #3300 +/- ##
=======================================
Coverage 81.71% 81.71%
=======================================
Files 556 556
Lines 74130 74141 +11
=======================================
+ Hits 60573 60583 +10
- Misses 13557 13558 +1
|
1c92407
into
oscar-system:master
* added graph_from_edges function * deleted useless lines * added optional argument additional_vertices for isolated vertices * fixed typo in Documentation * changed graph_from_edges to follow LarsK suggestions * fixed documentation * n_vertices < n_needed now throws an error
Added a function that creates a graph from a set of edges.