Added constructors to create graphs from a vector or an iterator of edges

[simonschoelly]

As mentioned in #875 , I have created the following constructors:

SimpleGraph(edge_list::Vector{SimpleGraphEdge{T}}) where T<:Integer
SimpleGraphFromIterator(iter)
SimpleDiGraph(edge_list::Vector{SimpleDiGraphEdge{T}}) where T<:Integer
SimpleDiGraphFromIterator(iter)

Some shortcomings of my approach:

1. SimpleGraph(edge_list) and SimpleDiGraph(edge_list) first evaluate how much memory to allocate. For this, they have to temporarily allocate some memory in the order of O(|V|).
2. The return type of SimpleGraphFromIterator(iter) and SimpleDiGraphFromIterator(iter) can not be inferred. The problem is, that one cannot easily infer the element type of a Generator. If one looks at how the constructor for Set is implemented, they can see that is somehow possible, but this is not trivial and goes beyond my knowledge of Julia's internals.
3. The constructors are conservative and assume that the input is neither sorted nor free of duplicates. As the constructors probably spend most of their time allocating memory, this should not be that big of a problem though.

[codecov]

Codecov Report

Merging #880 into master will increase coverage by 0.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #880      +/-   ##
==========================================
+ Coverage   99.67%   99.69%   +0.01%     
==========================================
  Files          60       60              
  Lines        2160     2281     +121     
==========================================
+ Hits         2153     2274     +121     
  Misses          7        7

[sbromberger]

More comments later. Thanks.

[sbromberger]

this check can be expensive. Might as well just sort and be done with it, or pass in a boolean argument to denote whether we can assume the vectors are sorted.

[simonschoelly]

I don't think calling issorted is that expensive; especially compared to sort! . When I wrote that function I used the profiler to check where the bottlenecks are and issort was definitely not the problem. unique! actually also calls this function, which is unnecessary in our case as we know that the vector must be sorted.

I could implement a flag to indicate that the input is sorted and otherwise correct but then the problem is that a user could actually produce an invalid graph. I think, if someone really need to optimize for the last bits of performance, then they are better of by creating fadjlist and badjlist themself. I tested that by writing a function that builds a complete graph and for 10^4 vertices I am a factor of 2.5 times faster than by creating a vector of edges and calling SimpleGraph with sorting disabled.

Then I wrote some code to see what the performance is, if we don't sort/don't check for sorted input:
Here are the results for 100 and 10^4 vertices:

julia> run_speed_tests(100; ntimes=50)
Sorted Input:
  SimpleGraph                  : mean == 0.000229s  std == 1.61e-5s
  SimpleGraph (no checks)       : mean == 0.00258s  std == 0.0174s
  SimpleGraph (always sort)    : mean == 0.000119s  std == 2.1e-5s
Shuffled Input:
  SimpleGraph                  : mean == 0.000307s  std == 2.2e-5s
  SimpleGraph (always sort)    : mean == 0.000325s  std == 2.03e-5s

julia> run_speed_tests(10000; ntimes=50)
Sorted Input:
  SimpleGraph                  : mean == 1.44s  std == 0.0452s
  SimpleGraph (no checks)       : mean == 1.16s  std == 0.0314s
  SimpleGraph (always sort)    : mean == 1.97s  std == 0.0348s
Shuffled Input:
  SimpleGraph                  : mean == 7.16s  std == 0.079s
  SimpleGraph (always sort)    : mean == 7.26s  std == 0.068s

• Sorted input constructs a complete graph from a sorted list of edges

• Shuffled Input constructs the same graph when the list is randomly shuffled

• Simple graph is the constructor form the pr

• Simple graph (no checks) assumes the input is sorted, has no duplicates and the vertices of each edge have values >= 1.

• Simple graph (always sort) does not check if the vertices are sorted and just sorts them

As you can see, if the input is random, then issort does not make the code much slower, at least not for large graphs. If the input is already sorted, then using issort is faster than just sorting. In this case, using a flag that indicates sorted input gives some performance improvements but as I said, I'm not sure if it is worth the potential errors that improper usage could create.

[sbromberger]

The numbers don't lie. Thank you for checking. issorted will be at worst an O(n) operation, right?

I agree that the performance difference is not significant enough to worry about at this point. Carry on!

[simonschoelly]

Yes, O(n) where n is the number of input edges.

[sbromberger]

You're calling cleanupedges! here, which sorts and dedups fadjlist and badjlist, but you've already assigned them to g. The issue is that the adjacency lists of a SimpleGraph are presumed to be sorted and deduplicated. So if they're unsorted/redundant at the time you assign them to g, then that's wrong. If they're sorted/unique, then you don't need to re-sort. Since this is likely modifying the backing array (and changing g.fadjlist which is a reference to it), it should probably come first just for clarity.

[sbromberger]

cc @mschauer, @somil55 and @jpfairbanks for reviews / comments, please?

[mschauer]

Nice. I'll have a look. Could you do performance comparisons with my implementation?

[simonschoelly]

Your solution is of course more general, so I had to write a wrapper around it:

using LightGraphs
using Random
using BenchmarkTools

function SimpleDiGraphAddEdges(edge_list::Vector{Tuple{T, T}}) where T<:Integer
    g = SimpleDiGraph{T}()
    add_edges!(g, edge_list)
    return g
end

Then I added some benchmarks for ordered and unordered lists of edges of a complete directed graph

el = [e for e in edges(CompleteDiGraph(n))];
el_tuple = Tuple.(el);
el_shuffle = shuffle(el);
el_shuffle_tuple = Tuple.(el_shuffle);

@benchmark SimpleDiGraph(el)
@benchmark SimpleDiGraphFromIterator(el)
@benchmark SimpleDiGraphAddeEdges(el_tuple)
@benchmark SimpleDiGraph(el_shuffle)
@benchmark SimpleDiGraphFromIterator(el_shuffle)
@benchmark SimpleDiGraph(el_shuffle_tuple)

Here are the median running times for n = 70 and n = 7000.

n = 70

SimpleDiGraph(el)                      :  58.939 μs
SimpleDiGraphFromIterator(el)          : 129.210 μs
SimpleDiGraphAddEdges(el_tuple)        : 204.505 μs
SimpleDiGraph(el_shuffle)                : 279.022 μs
SimpleDiGraphFromIterator(el_shuffle)    : 360.221 μs
SimpleDiGraphAddEdges(el_shuffle_tuple)  : 384.589 μs

n = 7000

SimpleDiGraph(el)                      : 1.568 s
SimpleDiGraphFromIterator(el)          : 2.531 s
SimpleDiGraphAddEdges(el_tuple)        : 3.382 s
SimpleDiGraph(el_shuffle)                : 7.542 s
SimpleDiGraphFromIterator(el_shuffle)    : 9.068 s
SimpleDiGraphAddEdges(el_shuffle_tuple)  : 9.444 s

So as long as the data is not ordered and an arbitrary iterator, your solution has the same running time as mine.
Instead of append!(g.fadjlist[s], d) you could have used push!(g.fadjlist[s], d) but I don't know if that would make any difference.

[mschauer]

Iterates twice over edge_list, this perhaps the right thing to do as it is known to be a vector.

[simonschoelly]

I have to do that in order to preallocate the adjacency list as this is much faster.

[mschauer]

Thank you.