Graphs.jl icon indicating copy to clipboard operation
Graphs.jl copied to clipboard

Add more convenience constructors

Open CarloLucibello opened this issue 3 years ago • 3 comments

The type GNNGraph from GraphNeuralNetworks.jl is a Graphs.AbstractGraphs and implements the corresponding interface, therefore it would be nice to make the following code work by providing an appropriate constructor:

julia> using GraphNeuralNetworks, Graphs

julia> g = GraphNeuralNetworks.rand_graph(10, 20)
GNNGraph:
    num_nodes = 10
    num_edges = 20

julia> g isa Graphs.AbstractGraph
true

julia> Graphs.SimpleGraph(g)
ERROR: MethodError: no method matching SimpleGraph(::GNNGraph{Tuple{Vector{Int64}, Vector{Int64}, Nothing}})
Closest candidates are:
  SimpleGraph(::Any, ::Array{Vector{T}, 1}) where T at ~/.julia/packages/Graphs/pxGmw/src/SimpleGraphs/simplegraph.jl:18
  SimpleGraph() at ~/.julia/packages/Graphs/pxGmw/src/SimpleGraphs/simplegraph.jl:50
  SimpleGraph(::SimpleGraph) at ~/.julia/packages/Graphs/pxGmw/src/SimpleGraphs/simplegraph.jl:115
  ...

Moreover, we could also have the following constructor:

julia> src, dest = edge_index(g)
([1, 1, 1, 1, 2, 2, 2, 5, 5, 5, 2, 3, 7, 10, 5, 6, 7, 6, 7, 9], [2, 3, 7, 10, 5, 6, 7, 6, 7, 9, 1, 1, 1, 1, 2, 2, 2, 5, 5, 5])

julia> SimpleGraph(src, dest)
ERROR: MethodError: no method matching SimpleGraph(::Vector{Int64}, ::Vector{Int64})
Closest candidates are:
  SimpleGraph(::Any, ::Array{Vector{T}, 1}) where T at ~/.julia/packages/Graphs/pxGmw/src/SimpleGraphs/simplegraph.jl:18

which is more concise and discoverable than

julia> SimpleGraph(Edge.(src,dest))
{10, 10} undirected simple Int64 graph

CarloLucibello avatar May 21 '22 17:05 CarloLucibello

Is it a bug though? Not rather a new feature?

I fully agree with your first point, there is also an older issue for that:: https://github.com/JuliaGraphs/Graphs.jl/issues/98

About the second part, I feel like we have too many constructors as it might start to get confusing, so I am in favor of also creating factory functions with different names, such as SimpleGraphFromIteratator that we had to create because of some type issue.

But we probably should also support tuples more. Then one could also write SimpleGraph(zip(src, dst)) in your second example.

simonschoelly avatar May 21 '22 18:05 simonschoelly

Is it a bug though? Not rather a new feature?

sorry, I accidentally clicked on the "bug" button when opening an issue and now I cannot remove the label.

About the second part, I feel like we have too many constructors

SimpleGraph(src::AbstractVector{<:Integer}, dst::AbstractVector{<:Integer}) 

is pretty natural and semantically unambiguous. It is something that you can find out by yourself without digging in the documentation.

SimpleGraph((src, dest)::Tuple{AbstractVector{<:Integer}, AbstractVector{<:Integer}}) 

is another option

CarloLucibello avatar May 22 '22 20:05 CarloLucibello

PR #301 tackles the first part of your request, can you take a look @CarloLucibello? PR #252 might address the second part but I'm not gonna merge that one yet

gdalle avatar Sep 14 '23 10:09 gdalle