JuliaGraphs / Graphs.jl

An optimized graphs package for the Julia programming language
http://juliagraphs.org/Graphs.jl/
Other
450 stars 87 forks source link

Add more convenience constructors #131

Open CarloLucibello opened 2 years ago

CarloLucibello commented 2 years ago

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
simonschoelly commented 2 years ago

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.

CarloLucibello commented 2 years ago

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

gdalle commented 10 months ago

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