Implementing DigraphEdgeConnectivity

[RheyaM]

Implemented two methods for calculating the EdgeConnectivity of a Digraph, one using Spanning Trees and one using Dominating Sets, based on the Algorithms detailed in: https://www.cse.msu.edu/~cse835/Papers/Graph_connectivity_revised.pdf

Additional functions: DigraphDominatingSet() and DigraphGetNeighbourhood(), for getting a Dominating Set of a digraph and getting the neighbourhood of a subset of vertices in a digraph, respectively.

[reiniscirpons]

Thanks for the implementation! I had some suggestions for the documentation and for improving the performance of the methods. Mainly more details in the docs and reworking some loops to implement more efficient DFS.

Some other things:

• The documentation is not currently linked in the manual. You should edit the correct doc/z-chapX.xml file to add this. For example, to get the DigraphDominatingSet documentation to show up you should add the <#Include Label="DigraphDominatingSet"> tag in the correct place of doc/z-chap4.xml. You can test this by seeing if ?DigraphDominatingSet shows up the docs after running DigraphsMakeDoc() in gap. Same applies for the other functions.
• Functions need to be more careful about symmetric vs non symmetric digraphs.
• You should re-run the benchmarks after making the performance based changes to the implementation. I think currently the benchmarks are skewed by the time it takes to find the dominating set/spanning tree, but after the reimplementation this time should become negligible, and it will be more about how many vertices need to be checked. So maybe hold-off on removing the methods for now.

Afterwards should be good to merge!

[reiniscirpons]

Suggested change

	This creates a dominating set of <A>digraph</A>, which is a subset of the vertices in
	<A>digraph</A> such that the neighbourhood of this subset of vertices is equal to all
	the vertices in <A>digraph</A>. </P>
	This function returns a <E>dominating set</E> of <A>digraph</A>,
	which is a subset of vertices whose neighbourhood consists of all
	vertices in <A>digraph</A>. <P/>

Some suggestions for rephrasing the sentence a bit. I think generally the first sentence of a function description should concisely describe what the function does in terms of what does the function expect of its arguments and what does the function return. Saying it "creates" something is not bad, but its maybe slightly unclear whether this is created internally and stored somewhere or returned by the function directly etc. Also the new paragraph attribute should be <P/>.

I think you should also clarify what a neighbourhood of a set of vertices is, this is not immediately clear.

[reiniscirpons]

Suggested change

	<#GAPDoc Label="DigraphGetNeighbourhood">
	<#GAPDoc Label="DigraphOutNeighbourhood">

I think omitting the Get would be better. There is no corresponding DigraphSetNeighbourhood function, so there is no risk of confusion.

Also, since this function is intended for directed graphs, you should specify whether it returns the set of out-neightbours or in-neighbours (and probably implement the in-neighbours version as well).

Alternatively, if the function is only intended for symmetric graphs, you should check for this explicitly in the function definition and also document it accordingly.

[reiniscirpons]

Suggested change

	This returns a record representing the edge connectivity of <A>digraph</A>.<P/>
	This function returns the edge connectivity of <A>digraph</A>.<P/>

This is very sparse on details. I think you should define what the edge-connectivity of a digraph is, it is not at all clear. Most users reading this function will probably be learning about edge connectivity for the first time, you shouldn't assume they are experts in graph theory (and even experts appreciate the reminder/clarification about what exactly we mean by edge connectivity).

As a rule of thumb, when writing, imagine you are writing the doc to explain it to yourself in the past, before you ever undertook this project.

Also, a record is a specific object is gap e.g. rec(a := 2, b:= 3) which stores a set of key-value pairs. This function just returns an integer not a record.

[reiniscirpons]

Suggested change

	It makes use of <C>DigraphMaximumFlow(<A>digraph</A>)</C>, by constructing an edge-weighted Digraph
	It makes use of <C>DigraphMaximumFlow(<A>digraph</A>)</C> function, by constructing an edge-weighted Digraph

It might also be a good idea to link to the doc of the DigraphMaximumFlow documentation, e.g.

See also <Ref Attr="DigraphMaximumFlow"/>.

[reiniscirpons]

This function is correct, but its performance could be improved. In particular, calling Union(X, Y) for two lists $X$ and $Y$ will take about $|X|+|Y|$ steps to compute, this is because, apart from anything, it creates a completely new list consisting of the union of $X$ and $Y$. Furthermore, calling Difference(X, Y) also takes about |X|+|Y| steps for similar reason.

Part of the issue is that the Union function creates a new list every time, instead of modifying the list neighbourhood, which costs a lot of time. Gap provides the UniteSet and IntersectSet functions (see https://docs.gap-system.org/doc/ref/chap21_mj.html#X7B3469CD7EFC1A87 ) which will modify the first argument in place, which would be better since you want to change the set of neighbours anyway. So something like

  for v in vertices do
    UniteSet(neighbourhood, OutNeighbours(digraph)[v]);
    DifferenceSet(neighbourhood, vertices);
  od;

would be a first step to improving the function.

But we can do better! Removing the vertices vertices doesn't have to happen every iteration, indeed we could just do this once at the very end.
Similarly, its not clear you need to maintain neighbourhood to be a set all the while, you could in theory let it have duplicate vertices and just remove them at the end. So, I would suggest something like

Suggested change

	neighbourhood := [];
	for v in vertices do
	neighbourhood := Difference(Union(neighbourhood,
	OutNeighbours(digraph)[v]), vertices);
	od;
	neighbourhood := [];
	for v in vertices do
	Append(neighbourhood, OutNeighbours(digraph)[v]);
	od;
	neighbourhood := Difference(Unique(neighbourhood), vertices);

Here the Append function modifies neighbourhood in place, so every loop you would only take about Length( OutNeighbours(digraph)[v]) doing this, instead of also paying the cost of the length of neighbourhood and vertices every time.

And then you can remove duplicate vertices using the Unique function, and then do the difference with vertices once at the end of the function.

[reiniscirpons]

Suggested change

	if DigraphNrVertices(digraph) = 1 then
	return 0;
	fi;
	if DigraphNrStronglyConnectedComponents(digraph) > 1 then
	return 0;
	fi;
	if DigraphNrVertices(digraph) = 1 or
	DigraphNrStronglyConnectedComponents(digraph) > 1 then
	return 0;
	fi;

Its better to join the two checks into one. Additionally, should the function look at strongly connected components or weakly connected components?

I think the edge-connectivity, as described in Chapter 2 of the notes by Abdol–Hossein Esfahanian, only applies to undirected graphs (i.e. symmetric digraphs). It could be applied to directed digraphs too, if we took the symmetric closure of the graph in this function. This could be called the "weak" edge connectivity for a directed graph. But I think in this case we need to check for DigraphNrConnectedComponents not DigraphNrStronglyConnectedComponents. The difference is that in the latter case, the graph Digraph([[1], []]) is not strongly connected, but it is weakly connected.

The method in Chapter 3 of the notes is a method for computing arc-connectivity, which is a notion for directed graph specifically.

I think doing the following would fix the method:

Suggested change

	if DigraphNrVertices(digraph) = 1 then
	return 0;
	fi;
	if DigraphNrStronglyConnectedComponents(digraph) > 1 then
	return 0;
	fi;
	if DigraphNrVertices(digraph) = 1 or
	DigraphNrConnectedComponents(digraph) > 1 then
	return 0;
	fi;
	digraph := DigraphSymmetricClosure(DigraphImmutableCopyIfMutable(D));

Here the DigraphImmutableCopyIfMutable makes sure we dont accidentally change D if its mutable, and DigraphSymmetricClosure takes the symmetric closure of the digraph.

[reiniscirpons]

Similar to the dominating set method, I think this could be made much more efficient by implementing a DFS and using a boolean list to keep track of the vertices that have been seen so far should suffice.

Additionally, you probably don't need to build the full spanning tree graph here, simply keeping track of which vertices were leaves (perhaps in a separate boolean list). This could be done by seeing if all the out-neighbours w of the current vertex v have seen[w] = true.

[reiniscirpons]

Would it make sense here to try the dominating set method and only use the simpler all-pairs method if that one fails too? This would make the logic more complicated so just a suggestion.

[reiniscirpons]

Suggested change

	if (sum < min or min = -1) then
	if sum < min or min = -1 then

Unlike in C++, the brackets are not required for conditionals in if statements.

[reiniscirpons]

This part of code is almost the same across the two EdgeConnectivity functions, so should be factored out into its own helper function. Alternatively, if the EdgeConnectivity or EdgeConnectivityDS functions get merged or one of them gets removed then this gets resolved too.