Skip to content
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

Add more convenience constructors #131

Open
CarloLucibello opened this issue May 21, 2022 · 3 comments
Open

Add more convenience constructors #131

CarloLucibello opened this issue May 21, 2022 · 3 comments
Labels
enhancement New feature or request

Comments

@CarloLucibello
Copy link
Contributor

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 CarloLucibello added the bug Something isn't working label May 21, 2022
@simonschoelly
Copy link
Contributor

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:: #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
Copy link
Contributor Author

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

@etiennedeg etiennedeg added enhancement New feature or request and removed bug Something isn't working labels May 24, 2022
@gdalle gdalle added this to the v1.9 milestone Jun 28, 2023
@gdalle
Copy link
Member

gdalle commented Sep 14, 2023

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 gdalle closed this as completed Sep 14, 2023
@gdalle gdalle reopened this Sep 14, 2023
@gdalle gdalle removed this from the v1.9 milestone Sep 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

4 participants