Behaviour of DigraphRemoveEdge/DigraphRemoveEdges is inconsistent

[flsmith]

Currently if digraph is immutable:

• DigraphRemoveEdges(digraph, [ ]) returns digraph, but
• DigraphRemoveEdges(digraph, [[src, ran]]) always returns an immutable copy of digraph even when [src, ran] is invalid, and
• DigraphRemoveEdge(digraph, [src, ran]) always returns an immutable copy of digraph even when [src, ran] is invalid

which seems slightly odd, do we want this behaviour?

[wilfwilson]

For these kinds of functions, I'm all for the operation returning the identical unchanged immutable digraph in the case that the argument is immutable and the operation doesn't want to change the digraph in any way.

I can't see why we should insist on creating on a new immutable copy in this case (which will take up new memory, and which will lose all of its stored attributes). If a user wants a new immutable copy, they can always ask for one explicitly.

(I also think DigraphRemoveEdge should cause an Error when the input is invalid.)

Any other opinions?

[james-d-mitchell]

Very late to the party, but I agree with @wilfwilson. If the digraph is not changed, then there's no need to copy it.

[saffronmciver]

This appears to also be the case for DigraphAddEdge, DigraphAddEdges, DigraphRemoveVertex, DigraphRemoveVertices, DigraphReverseEdge, DigraphReverseEdges, OnDigraphs and QuotientDigraph.

One related potential issue is in DigraphAddEdges and DigraphRemoveEdges (mutable), if the list of edges contains edges that would be valid to add or remove, but invalid edges later in the list, the mutable graph is edited in an unpredictable way (so the first edges that are valid are added / removed, but the operation stops once any invalid edge is found).

Example:

D := DigraphByEdges([[1, 2]]);
D := DigraphAddEdges(D, [[2, 1], [3, 2], [2, 2]]); 
DigraphEdges(D);
D2 := DigraphByEdges(IsMutableDigraph, [[1, 2]]);
D2 := DigraphAddEdges(D2, [[2, 1], [3, 2], [2, 2]]);
DigraphEdges(D2);

D2 will have the edge [2, 1] while D1 does not.