New interface for GraphsExtensions.similar_graph; misc bug fixes.

[jack-dunham]

This PR overhauls the way the GraphsExtensions.similar_graph function behaves, and includes some minor bug fixes and refactors.

New similar_graph interface

Essentially, the base case is now the three argument method, where one provides both vertices and edges. This defaults to constructing a concrete NamedGraph or NamedDiGraph depending on the IsDirected trait of the input graph.

similar_graph(::AbstractGraph::!(IsDirected), vertices, edges) # -> NamedGraph
similar_graph(::AbstractGraph::IsDirected, vertices, edges) # -> NamedDiGraph

If edges are not provided, then the function attempts to construct a similar graph to the input graph, but with the provided vertices and no edges. If no vertices or edges are provided, then the output graph will be a similar graph with the same vertices and edges. For a given subtype of AbstractNamedGraph, one should overload the three argument method only (if a different return type is desired).

Type domain

If instead a graph type is provided as the first argument to similar_graph, then the constructor is called by default:

similar_graph(T::Type{<:AbstractGraph})  = T()
similar_graph(T::Type{<:AbstractGraph}, vertices)  = T(vertices)
similar_graph(T::Type{<:AbstractGraph}, vertices, edges)  = T(vertices, edges)

Note, unlike the case where a graph value is provided, the one and two argument methods are not defined in terms of the three argument method. This is to accommodate cases where a given AbstractGraph may have fixed edges, say (a notable example would be a tree graph). In that case, it is assumes there exists a constructor T(vertices). As the method

similar_graph(T::Type{<:AbstractGraph}, vertices)  = T(vertices)

may or may not return an edgeless graph, depending on the definition of the constructor, there exists the function

edgeless_graph(T::Type{<:AbstractGraph}, vertices)

that attempts to explicitly a construct an edgeless graph, thus throwing an error when this is not possible. Likewise, there exists a function

empty_graph(T::Type{<:AbstractGraph})

for explicitly constructing a graph of type T with no vertices or edges.

If your graph type MyGraph has freedom over vertices and edges, all three type-based method can be overloaded simultaneously using the followings signature:

similar_graph(T::Type{<:MyGraph}, vertices = [], edges = [])  = ...

[mtfishman]

Likewise, there exists a function

empty_graph(T::Type{<:AbstractGraph}, vertices)

for explicitly constructing a graph of type T with no vertices or edges.

Is that supposed to be:

empty_graph(T::Type{<:AbstractGraph})

?

[mtfishman]

If no vertices or edges are provided, then the output graph will be a similar empty graph.

I thought we had decided it would be a graph with the same vertices and edges as the input graph (in the case where a graph instance is input)? That would be analogous to similar of SparseArrays:

julia> using SparseArrays

julia> a = sprandn(5, 5, 0.5)
5×5 SparseMatrixCSC{Float64, Int64} with 13 stored entries:
   ⋅          ⋅        -0.313373   ⋅     ⋅ 
   ⋅         0.360588  -2.40419    ⋅   -0.885288
 -0.538721  -1.59639     ⋅         ⋅    2.41016
 -1.25074     ⋅        -0.652935   ⋅     ⋅ 
 -2.0638    -0.512399   2.09563    ⋅   -1.35611

julia> similar(a)
5×5 SparseMatrixCSC{Float64, Int64} with 13 stored entries:
  ⋅             ⋅            2.27785e-314   ⋅    ⋅ 
  ⋅            2.76437e-314  2.27785e-314   ⋅   2.27785e-314
 2.27785e-314  2.20064e-314   ⋅             ⋅   2.76437e-314
 2.27785e-314   ⋅            2.27785e-314   ⋅    ⋅ 
 2.27785e-314  2.27785e-314  2.27785e-314   ⋅   2.76437e-314

EDIT: I think I see that is how it is implemented, so maybe it is just that the description in the first post is outdated.

[mtfishman]

This looks strange to me, shouldn't it be a NamedGraph with the set of vertices vertices? I guess this is specialized on AbstractNamedGraph somewhere else?

I think this is a case where we should follow the principle that the input indices (in this case, vertices) guide the output type. Since vertices is very general here, we can't determine based on the types of the inputs that the output should be a SimpleGraph (it should only be a SimpleGraph if the vertices are a one-based range, or maybe an integer specifying the number of vertices). Maybe we could have versions like:

@traitfn function similar_graph(graph::AbstractGraph::(!IsDirected), nvertices::Integer, edges)
    new_graph = SimpleGraph(nvertices)
    add_edges!(new_graph, edges)
    return new_graph
end

@traitfn function similar_graph(graph::AbstractGraph::(!IsDirected), vertices::Base.OneTo, edges)
    return similar_graph(graph, length(vertices), edges)
end

@traitfn function similar_graph(graph::AbstractGraph::(!IsDirected), vertices, edges)
    # NamedGraph output
end

but I think we could keep it simpler for now and add the nvertices::Integer and vertices::Base.OneTo later if needed.

[jack-dunham]

The reason I don't define:

@traitfn function similar_graph(graph::AbstractGraph::(!IsDirected), vertices, edges)
    # NamedGraph output
end

is because GraphsExtensions, where similar_graph is defined, shouldn't depend on NamedGraphs (although I see now that the Project.toml in that library does have NamedGraphs as a dependency (should it?).). We also can't define that method in NamedGraph itself as it would be type piracy.

As for the other points, I agree that the vertices argument should be explicitly restricted to ::Int and ::Base.OneTo (thanks).

[mtfishman]

If we are thinking about it in terms of dependency structure, the issue is that even if we design it like this:

@traitfn function similar_graph(graph::AbstractGraph::(!IsDirected), vertices::Base.OneTo, edges)
    # SimpleGrade output
end

@traitfn function similar_graph(graph::AbstractGraph::(!IsDirected), vertices, edges)
    # NamedGraph output
end

then the second one is type piracy. Can similar_graph be a NamedGraphs function? Do we actually need it in GraphsExtensions? If we do, we could just define two versions of it, one in GraphsExtensions and one in NamedGraphs. I think this is bumping up against the awkwardness of dealing with both SimpleGraphs and NamedGraphs in a single interface.

[mtfishman]

(The version in GraphsExtensions could be called similar_simplegraph to distinguish the two versions.)

[jack-dunham]

I think that is sensible. I've checked in GraphsExtensions and the only two functions that use similar_graph are directed_graph and undirected_graph. We can just define these two again in NamedGraphs to use NamedGraphs.similar_graph instead of similar_simplegraph.

[mtfishman]

Same comment as above.

[mtfishman]

Do we need these or can we just use similar_graph? The main motivation is to try to simplify this PR and the interface.

[jack-dunham]

The functions are not needed, but I do think it will make the code clearer long term to have:

new_graph = empty_graph(graph)
# ... some code that assume `new_graph` is definitely empty

compared to:

new_graph = similar_graph(graph, [])
# ... some code that assume `new_graph` is definitely empty

where someone reading the code out of context (i.e. me) might not know (or remember) exactly what similar_graph(graph, []) does and as a result, may find the subsequent code harder to interpret.

[mtfishman]

That's a fair point, however I do think we have to be careful about the definition of these functions. I would expect edgeless_graph and empty_graph to preserve the graph type, like empty preserves the array type. However, the current definitions through similar_graph don't necessarily preserve the graph type. So I would expect the implementations would be more like:

edgeless_graph(g::AbstractGraph) = rem_edges!(copy(g), edges(g))
empty_graph(g::AbstractGraph) = rem_vertices!(copy(g), vertices(g))

which would also justify having them as separate functions.

[jack-dunham]

If no vertices or edges are provided, then the output graph will be a similar empty graph.

I thought we had decided it would be a graph with the same vertices and edges as the input graph (in the case where a graph instance is input)? That would be analogous to similar of SparseArrays:

julia> using SparseArrays

julia> a = sprandn(5, 5, 0.5)
5×5 SparseMatrixCSC{Float64, Int64} with 13 stored entries:
   ⋅          ⋅        -0.313373   ⋅     ⋅ 
   ⋅         0.360588  -2.40419    ⋅   -0.885288
 -0.538721  -1.59639     ⋅         ⋅    2.41016
 -1.25074     ⋅        -0.652935   ⋅     ⋅ 
 -2.0638    -0.512399   2.09563    ⋅   -1.35611

julia> similar(a)
5×5 SparseMatrixCSC{Float64, Int64} with 13 stored entries:
  ⋅             ⋅            2.27785e-314   ⋅    ⋅ 
  ⋅            2.76437e-314  2.27785e-314   ⋅   2.27785e-314
 2.27785e-314  2.20064e-314   ⋅             ⋅   2.76437e-314
 2.27785e-314   ⋅            2.27785e-314   ⋅    ⋅ 
 2.27785e-314  2.27785e-314  2.27785e-314   ⋅   2.76437e-314

EDIT: I think I see that is how it is implemented, so maybe it is just that the description in the first post is outdated.

Sorry this is my mistake in the post description. It should construct a graph with the same vertices and edges. I've updated the head comment.

[mtfishman]

Suggested change

	partition_vertices, rem_edges!, rename_vertices, similar_edgeless_graph, similar_graph,
	partition_vertices, rem_edges!, rename_vertices, similar_graph,

Looks like a stale import.