-
Notifications
You must be signed in to change notification settings - Fork 1
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
Interface as simple as possible #13
base: main
Are you sure you want to change the base?
Changes from all commits
448250e
7aca557
dae585d
dd4c6b0
3e9e4af
bac93dc
685b90d
4202619
c001ac9
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,13 @@ | ||
# Concrete implementations | ||
|
||
## `SimpleGraphs` | ||
|
||
```@autodocs | ||
Modules = [GraphsBase.SimpleGraphs] | ||
``` | ||
|
||
## `SimpleWeightedGraphs` | ||
|
||
```@autodocs | ||
Modules = [GraphsBase.SimpleWeightedGraphs] | ||
``` |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,61 @@ | ||
# Interface | ||
|
||
## Edge | ||
|
||
```@docs | ||
AbstractEdge | ||
src | ||
dst | ||
weight | ||
Base.reverse | ||
``` | ||
|
||
## Graph (required) | ||
|
||
```@docs | ||
AbstractGraph | ||
is_directed | ||
vertices | ||
out_edges | ||
in_edges | ||
``` | ||
|
||
## Graph (optional) | ||
|
||
```@docs | ||
nv | ||
ne | ||
has_vertex | ||
has_edge | ||
has_self_loops | ||
edges | ||
out_neighbors | ||
in_neighbors | ||
create_vertex_container | ||
create_edge_container | ||
``` | ||
|
||
## Graph (modification) | ||
|
||
```@docs | ||
add_vertex! | ||
rm_vertex! | ||
add_edge! | ||
rm_edge! | ||
``` | ||
|
||
## Element types | ||
|
||
```@docs | ||
Base.eltype | ||
edgetype | ||
weighttype | ||
``` | ||
|
||
## Checks | ||
|
||
```@docs | ||
GraphsBase.check_comparable_interface | ||
GraphsBase.check_edge_interface | ||
GraphsBase.check_graph_interface | ||
``` |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,14 @@ | ||
module SimpleGraphs | ||
|
||
using DocStringExtensions | ||
using GraphsBase | ||
|
||
export SimpleEdge | ||
export SimpleGraph | ||
export SimpleDiGraph | ||
|
||
include("simpleedge.jl") | ||
include("simplegraph.jl") | ||
include("simpledigraph.jl") | ||
|
||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,29 @@ | ||
""" | ||
$(TYPEDEF) | ||
|
||
A type representing a directed graph with unweighted, non-multiple edges. | ||
|
||
# Fields | ||
|
||
$(TYPEDFIELDS) | ||
""" | ||
mutable struct SimpleDiGraph{T<:Integer} <: AbstractGraph{T,SimpleEdge{T}} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why is it mutable? I guess to allow in-place modification of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. presumably but it's kinda sad, I'm curious if there's a better way There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. that was my thought at first, and I think I suggested it somewhere but got corrected, if only I could find the issue... then again, maybe it's time to start fresh! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I guess in newer versions of Julia you can mark |
||
"Number of edges" | ||
ne::Int | ||
"Forward adjacency list such that `F[v]` contains the out-neighbors of vertex `v`" | ||
fadjlist::Vector{Vector{T}} | ||
"Backward adjacency list such that `B[u]` contains the in-neighbors of vertex `v`" | ||
badjlist::Vector{Vector{T}} | ||
end | ||
|
||
GraphsBase.is_directed(::Type{<:SimpleDiGraph}) = true | ||
|
||
GraphsBase.vertices(g::SimpleDiGraph{T}) where {T} = one(T):T(length(g.fadjlist)) | ||
|
||
function GraphsBase.out_edges(g::SimpleDiGraph{T}, u) where {T} | ||
return (SimpleEdge{T}(u, v) for v in g.fadjlist[u]) | ||
end | ||
|
||
function GraphsBase.in_edges(g::SimpleDiGraph{T}, v) where {T} | ||
return (SimpleEdge{T}(u, v) for u in g.badjlist[v]) | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,24 @@ | ||
""" | ||
$(TYPEDEF) | ||
|
||
A type representing an unweighted directed edge. | ||
|
||
# Fields | ||
|
||
$(TYPEDFIELDS) | ||
""" | ||
struct SimpleEdge{T<:Integer} <: AbstractEdge{T,Int} | ||
"Source of the edge" | ||
src::T | ||
"Destination of the edge" | ||
dst::T | ||
end | ||
|
||
Base.Tuple(e::SimpleEdge) = (e.src, e.dst) | ||
Base.isless(e1::SimpleEdge, e2::SimpleEdge) = isless(Tuple(e1), Tuple(e2)) | ||
Base.:(==)(e1::SimpleEdge, e2::SimpleEdge) = Tuple(e1) == Tuple(e2) | ||
|
||
GraphsBase.src(e::SimpleEdge) = e.src | ||
GraphsBase.dst(e::SimpleEdge) = e.dst | ||
GraphsBase.weight(e::SimpleEdge) = 1 | ||
Base.reverse(e::SimpleEdge) = SimpleEdge(e.dst, e.src) | ||
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,25 @@ | ||
""" | ||
$(TYPEDEF) | ||
|
||
A type representing an undirected graph with unweighted, non-multiple edges. | ||
|
||
# Fields | ||
|
||
$(TYPEDFIELDS) | ||
""" | ||
mutable struct SimpleGraph{T<:Integer} <: AbstractGraph{T,SimpleEdge{T}} | ||
"""Number of edges""" | ||
ne::Int | ||
"Adjacency list such that `L[v]` contains the neighbors of vertex `v`" | ||
adjlist::Vector{Vector{T}} | ||
end | ||
|
||
GraphsBase.is_directed(::Type{<:SimpleGraph}) = false | ||
|
||
GraphsBase.vertices(g::SimpleGraph{T}) where {T} = one(T):T(length(g.adjlist)) | ||
|
||
function GraphsBase.out_edges(g::SimpleGraph{T}, u) where {T} | ||
return (SimpleEdge{T}(u, v) for v in g.adjlist[u]) | ||
end | ||
|
||
GraphsBase.in_edges(g::SimpleGraph, v) = out_edges(g, v) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What about Something I found slightly annoying in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,15 @@ | ||
module SimpleWeightedGraphs | ||
|
||
using DocStringExtensions | ||
using SparseArrays | ||
using GraphsBase | ||
|
||
export SimpleWeightedEdge | ||
export SimpleWeightedGraph | ||
export SimpleWeightedDiGraph | ||
|
||
include("simpleweightededge.jl") | ||
include("simpleweightedgraph.jl") | ||
include("simpleweighteddigraph.jl") | ||
|
||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,27 @@ | ||
""" | ||
$(TYPEDEF) | ||
|
||
A type representing a directed graph with weighted, non-multiple edges. | ||
|
||
# Fields | ||
|
||
$(TYPEDFIELDS) | ||
""" | ||
struct SimpleWeightedDiGraph{T<:Integer,W} <: AbstractGraph{T,SimpleWeightedEdge{T,W}} | ||
"Transposed weighted adjacency matrix: `weights[v, u]` contains the weight of edge `(u, v)`" | ||
weights::SparseMatrixCSC{W,T} | ||
end | ||
|
||
GraphsBase.is_directed(::Type{<:SimpleWeightedDiGraph}) = true | ||
GraphsBase.vertices(g::SimpleWeightedDiGraph{T}) where {T} = one(T):T(size(g.weights, 1)) | ||
|
||
function GraphsBase.out_edges(g::SimpleWeightedDiGraph{T}, u) where {T} | ||
A = g.weights | ||
return (SimpleWeightedEdge{T}(u, A.rowval[i], A.nzval[i]) for i in nzrange(A, u)) | ||
end | ||
|
||
function GraphsBase.in_edges(g::SimpleWeightedDiGraph, v) | ||
A = g.weights | ||
a = A[v, :] | ||
return (SimpleWeightedEdge{T}(u, v, a[u]) for u in a.nzind) | ||
end | ||
Comment on lines
+23
to
+27
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this points to a challenge of coupling metadata with edges. What if your graph requires some non-trivial calculation to access metadata (I can definitely think of situations where that is the case)? Every time you ask for an edge, you would need to perform that computation, even though most graph functions don't need the metadata and probably just want the incident vertices. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,26 @@ | ||
""" | ||
$(TYPEDEF) | ||
|
||
A type representing a weighted directed edge. | ||
|
||
# Fields | ||
|
||
$(TYPEDFIELDS) | ||
""" | ||
struct SimpleWeightedEdge{T<:Integer,W} <: AbstractEdge{T,W} | ||
"Source of the edge" | ||
src::T | ||
"Destination of the edge" | ||
dst::T | ||
"Weight of the edge" | ||
weight::W | ||
end | ||
|
||
Base.Tuple(e::SimpleWeightedEdge) = (e.src, e.dst, e.weight) | ||
Base.isless(e1::SimpleWeightedEdge, e2::SimpleWeightedEdge) = isless(Tuple(e1), Tuple(e2)) | ||
Base.:(==)(e1::SimpleWeightedEdge, e2::SimpleWeightedEdge) = Tuple(e1) == Tuple(e2) | ||
|
||
GraphsBase.src(e::SimpleWeightedEdge) = e.src | ||
GraphsBase.dst(e::SimpleWeightedEdge) = e.dst | ||
GraphsBase.weight(e::SimpleWeightedEdge) = e.weight | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We have examples where the weight would be determined in some potentially non-trivial way from vertex metadata, so it doesn't seem so natural to store it on an edge. What about allowing With the current design the only way to customize the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. sorry for the laconic answers, I'm just opening issues as I go to keep track of your ideas in a more discoverable location There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Makes sense, thanks. I'm sure these same issues/questions have been brought up before, sorry for rehashing things. It's definitely more helpful to look at a concrete code proposal rather than longer abstract discussions, so thanks for making this PR! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @etiennedeg deserves more credit, my PR is a simplified, structured version of his |
||
Base.reverse(e::SimpleWeightedEdge) = SimpleEdge(e.dst, e.src, e.weight) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Guess this should be |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,23 @@ | ||
""" | ||
$(TYPEDEF) | ||
|
||
A type representing an undirected graph with weighted, non-multiple edges. | ||
|
||
# Fields | ||
|
||
$(TYPEDFIELDS) | ||
""" | ||
struct SimpleWeightedGraph{T<:Integer,W} <: AbstractGraph{T,SimpleWeightedEdge{T,W}} | ||
"Symmetric weighted adjacency matrix: `weights[u, v]` and `weights[v, u]` both contain the weight of edge `{u, v}`" | ||
weights::SparseMatrixCSC{W,T} | ||
end | ||
|
||
GraphsBase.is_directed(::Type{<:SimpleWeightedGraph}) = false | ||
GraphsBase.vertices(g::SimpleWeightedGraph{T}) where {T} = one(T):T(size(g.weights, 1)) | ||
|
||
function GraphsBase.out_edges(g::SimpleWeightedGraph{T}, u) where {T} | ||
A = g.weights | ||
return (SimpleWeightedEdge{T}(u, A.rowval[i], A.nzval[i]) for i in nzrange(A, u)) | ||
end | ||
|
||
GraphsBase.in_edges(g::SimpleWeightedGraph, v) = out_edges(g, v) |
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 really wish these horrible names would be removed or at least deprecated...
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.
nvertices
andnedges
?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.
That would at least fit with
ndigits
,nfields
,ndims
.(Personally I've come to loath all these abbreviations -- they are super nice if you bang away on experiments in the REPL. But write code with them and come back after a year and try to read it, and it looks like gobbledygook. Esp. if you have to context switch between multiple languages and libraries... So I am now tending into the more "radical" direction of preferring names like
num_vertices
or evennumber_of_vertices
. But I realize I am probably in the minority on this, which is fine, but I figure I should at least mention it once before staying silent forever after ;-). Anyhow, thank you for working on this ).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.
Yeah,
nvertices
andnedges
seems decent.