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

Interface as simple as possible #13

Draft
wants to merge 9 commits into
base: main
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions Project.toml
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,14 @@ uuid = "ad2ac648-372e-45be-9d57-a550431b71c3"
authors = ["JuliaGraphs contributors"]
version = "0.1.0-DEV"

[deps]
DocStringExtensions = "ffbed154-4ef7-542d-bbb7-c09d3a79fcae"
SimpleTraits = "699a6c99-e7fa-54fc-8d76-47d257e15c1d"
SparseArrays = "2f01184e-e22b-5df5-ae63-d93ebab69eaf"

[compat]
DocStringExtensions = "0.9"
SimpleTraits = "0.9"
julia = "1.6"

[extras]
Expand Down
6 changes: 5 additions & 1 deletion docs/make.jl
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,11 @@ makedocs(;
canonical="https://juliagraphs.org/GraphsBase.jl",
assets=String[],
),
pages=["Home" => "index.md"],
pages=[
"Home" => "index.md",
"Interface" => "interface.md",
"Implementations" => "implementations.md",
],
)

deploydocs(;
Expand Down
13 changes: 13 additions & 0 deletions docs/src/implementations.md
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]
```
6 changes: 2 additions & 4 deletions docs/src/index.md
Original file line number Diff line number Diff line change
Expand Up @@ -6,10 +6,8 @@ CurrentModule = GraphsBase

Documentation for [GraphsBase](https://github.com/JuliaGraphs/GraphsBase.jl).

## API reference

```@autodocs
Modules = [GraphsBase]
```@docs
GraphsBase
```

## Index
Expand Down
61 changes: 61 additions & 0 deletions docs/src/interface.md
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
Comment on lines +26 to +27

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...

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nvertices and nedges?

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 even number_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 ).

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, nvertices and nedges seems decent.

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
```
29 changes: 29 additions & 0 deletions src/GraphsBase.jl
Original file line number Diff line number Diff line change
Expand Up @@ -5,4 +5,33 @@ The basic interface and graph types for the JuliaGraphs ecosystem.
"""
module GraphsBase

using DocStringExtensions
using SimpleTraits
using SparseArrays

export AbstractEdge, check_edge_interface
export eltype, weighttype
export src, dst, weight

export AbstractGraph, check_graph_interface
export edgetype
export is_directed
export vertices
export edges, out_edges, in_edges
export nv, ne
export has_vertex, has_edge, has_self_loops
export out_neighbors, in_neighbors
export create_vertex_container, create_edge_container
export add_vertex!, rm_vertex!, add_edge!, rm_edge!

include("interface/utils.jl")
include("interface/abstractedge.jl")
include("interface/abstractgraph.jl")
include("interface/optional.jl")
include("interface/modification.jl")

include("SimpleGraphs/SimpleGraphs.jl")

include("SimpleWeightedGraphs/SimpleWeightedGraphs.jl")

end
14 changes: 14 additions & 0 deletions src/SimpleGraphs/SimpleGraphs.jl
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
29 changes: 29 additions & 0 deletions src/SimpleGraphs/simpledigraph.jl
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}}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is it mutable? I guess to allow in-place modification of ne?

Copy link
Member Author

@gdalle gdalle Sep 28, 2023

Choose a reason for hiding this comment

The 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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe ne::Base.RefValue{Int}?

Copy link
Member Author

Choose a reason for hiding this comment

The 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!

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess in newer versions of Julia you can mark fadjlist and badjlist as const so it may not matter conceptually either way. I think I found using Base.RefValue was a bit slower in practice compared to mutating a field of a mutable.

"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

Check warning on line 19 in src/SimpleGraphs/simpledigraph.jl

View check run for this annotation

Codecov / codecov/patch

src/SimpleGraphs/simpledigraph.jl#L19

Added line #L19 was not covered by tests

GraphsBase.vertices(g::SimpleDiGraph{T}) where {T} = one(T):T(length(g.fadjlist))

Check warning on line 21 in src/SimpleGraphs/simpledigraph.jl

View check run for this annotation

Codecov / codecov/patch

src/SimpleGraphs/simpledigraph.jl#L21

Added line #L21 was not covered by tests

function GraphsBase.out_edges(g::SimpleDiGraph{T}, u) where {T}
return (SimpleEdge{T}(u, v) for v in g.fadjlist[u])

Check warning on line 24 in src/SimpleGraphs/simpledigraph.jl

View check run for this annotation

Codecov / codecov/patch

src/SimpleGraphs/simpledigraph.jl#L23-L24

Added lines #L23 - L24 were not covered by tests
end

function GraphsBase.in_edges(g::SimpleDiGraph{T}, v) where {T}
return (SimpleEdge{T}(u, v) for u in g.badjlist[v])

Check warning on line 28 in src/SimpleGraphs/simpledigraph.jl

View check run for this annotation

Codecov / codecov/patch

src/SimpleGraphs/simpledigraph.jl#L27-L28

Added lines #L27 - L28 were not covered by tests
end
24 changes: 24 additions & 0 deletions src/SimpleGraphs/simpleedge.jl
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)

Check warning on line 19 in src/SimpleGraphs/simpleedge.jl

View check run for this annotation

Codecov / codecov/patch

src/SimpleGraphs/simpleedge.jl#L17-L19

Added lines #L17 - L19 were not covered by tests

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)

Check warning on line 24 in src/SimpleGraphs/simpleedge.jl

View check run for this annotation

Codecov / codecov/patch

src/SimpleGraphs/simpleedge.jl#L21-L24

Added lines #L21 - L24 were not covered by tests
25 changes: 25 additions & 0 deletions src/SimpleGraphs/simplegraph.jl
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

Check warning on line 17 in src/SimpleGraphs/simplegraph.jl

View check run for this annotation

Codecov / codecov/patch

src/SimpleGraphs/simplegraph.jl#L17

Added line #L17 was not covered by tests

GraphsBase.vertices(g::SimpleGraph{T}) where {T} = one(T):T(length(g.adjlist))

Check warning on line 19 in src/SimpleGraphs/simplegraph.jl

View check run for this annotation

Codecov / codecov/patch

src/SimpleGraphs/simplegraph.jl#L19

Added line #L19 was not covered by tests

function GraphsBase.out_edges(g::SimpleGraph{T}, u) where {T}
return (SimpleEdge{T}(u, v) for v in g.adjlist[u])

Check warning on line 22 in src/SimpleGraphs/simplegraph.jl

View check run for this annotation

Codecov / codecov/patch

src/SimpleGraphs/simplegraph.jl#L21-L22

Added lines #L21 - L22 were not covered by tests
end

GraphsBase.in_edges(g::SimpleGraph, v) = out_edges(g, v)

Check warning on line 25 in src/SimpleGraphs/simplegraph.jl

View check run for this annotation

Codecov / codecov/patch

src/SimpleGraphs/simplegraph.jl#L25

Added line #L25 was not covered by tests

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about reverse.(out_edges(g, v))?

Something I found slightly annoying in Graphs.jl was that SimpleEdge was sometimes treated as directed and sometimes not, which leads to weird corner cases when designing code that is generic between undirected and directed graphs. Ideally, I think an undirected graph should act as much like a directed graph that has edges in both directions between vertices that have edges. So from that perspective it may be helpful to have in_edges and out_edges output edges with reverse directions.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#16

15 changes: 15 additions & 0 deletions src/SimpleWeightedGraphs/SimpleWeightedGraphs.jl
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
27 changes: 27 additions & 0 deletions src/SimpleWeightedGraphs/simpleweighteddigraph.jl
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))

Check warning on line 16 in src/SimpleWeightedGraphs/simpleweighteddigraph.jl

View check run for this annotation

Codecov / codecov/patch

src/SimpleWeightedGraphs/simpleweighteddigraph.jl#L15-L16

Added lines #L15 - L16 were not covered by tests

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))

Check warning on line 20 in src/SimpleWeightedGraphs/simpleweighteddigraph.jl

View check run for this annotation

Codecov / codecov/patch

src/SimpleWeightedGraphs/simpleweighteddigraph.jl#L18-L20

Added lines #L18 - L20 were not covered by tests
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)

Check warning on line 26 in src/SimpleWeightedGraphs/simpleweighteddigraph.jl

View check run for this annotation

Codecov / codecov/patch

src/SimpleWeightedGraphs/simpleweighteddigraph.jl#L23-L26

Added lines #L23 - L26 were not covered by tests
end
Comment on lines +23 to +27

Choose a reason for hiding this comment

The 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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#14

26 changes: 26 additions & 0 deletions src/SimpleWeightedGraphs/simpleweightededge.jl
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)

Check warning on line 21 in src/SimpleWeightedGraphs/simpleweightededge.jl

View check run for this annotation

Codecov / codecov/patch

src/SimpleWeightedGraphs/simpleweightededge.jl#L19-L21

Added lines #L19 - L21 were not covered by tests

GraphsBase.src(e::SimpleWeightedEdge) = e.src
GraphsBase.dst(e::SimpleWeightedEdge) = e.dst
GraphsBase.weight(e::SimpleWeightedEdge) = e.weight
Copy link

@mtfishman mtfishman Sep 28, 2023

Choose a reason for hiding this comment

The 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 weight(g::AbstractGraph, e::AbstractEdge)? It could then have a fallback weight(e::AbstractEdge) in the cases when it can be determined just from an edge.

With the current design the only way to customize the weight function is by defining a new edge type, which doesn't seem so practical in general (i.e. I would want to customize by graph type or graph instance, say by storing a weight function as a field inside the graph type which I access when overloading weight).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#14

Copy link
Member Author

Choose a reason for hiding this comment

The 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

Choose a reason for hiding this comment

The 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!

Copy link
Member Author

Choose a reason for hiding this comment

The 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)

Check warning on line 26 in src/SimpleWeightedGraphs/simpleweightededge.jl

View check run for this annotation

Codecov / codecov/patch

src/SimpleWeightedGraphs/simpleweightededge.jl#L23-L26

Added lines #L23 - L26 were not covered by tests

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Guess this should be SimpleWeightedEdge(...)?

23 changes: 23 additions & 0 deletions src/SimpleWeightedGraphs/simpleweightedgraph.jl
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))

Check warning on line 16 in src/SimpleWeightedGraphs/simpleweightedgraph.jl

View check run for this annotation

Codecov / codecov/patch

src/SimpleWeightedGraphs/simpleweightedgraph.jl#L15-L16

Added lines #L15 - L16 were not covered by tests

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))

Check warning on line 20 in src/SimpleWeightedGraphs/simpleweightedgraph.jl

View check run for this annotation

Codecov / codecov/patch

src/SimpleWeightedGraphs/simpleweightedgraph.jl#L18-L20

Added lines #L18 - L20 were not covered by tests
end

GraphsBase.in_edges(g::SimpleWeightedGraph, v) = out_edges(g, v)
Loading