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.

Sign up for GitHub

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

Behaviour of DigraphRemoveEdge/DigraphRemoveEdges is inconsistent #260 #723

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

Behaviour of DigraphRemoveEdge/DigraphRemoveEdges is inconsistent #260 #723

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
2492f10
Edit functions to not return unnecessary copies
Feb 5, 2025
6e704ab
Fix unbound local variable
saffronmciver Feb 5, 2025
cc2feb9
Fix gaplint errors
saffronmciver Feb 5, 2025
b7f59d1
Remove now incorrect test, and corrected DigraphRemoveEdge documentation
saffronmciver Feb 5, 2025
a15fbe5
Merge branch 'digraphs:main' into consistent-remove-edge
saffronmciver Feb 7, 2025
07bfb34
Remove further unnecessary copies
saffronmciver Feb 12, 2025
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
4 changes: 2 additions & 2 deletions doc/oper.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -603,7 +603,7 @@ true
<P/>

Note that if <A>digraph</A> belongs to <Ref Filt="IsImmutableDigraph"/>,
then a new copy of <A>digraph</A> will be returned even if <A>edge</A> or
then a new copy of <A>digraph</A> will not be returned if <A>edge</A> or
<A>[src, ran]</A> does not define an edge of <A>digraph</A>.<P/>

<Example><![CDATA[
Expand All @@ -615,7 +615,7 @@ gap> new := DigraphRemoveEdge(D, [25000, 2]);;
gap> new = D;
true
gap> IsIdenticalObj(new, D);
false
true
gap> D := DigraphMutableCopy(D);;
gap> new := DigraphRemoveEdge(D, 2500, 2);;
gap> IsIdenticalObj(new, D);
Expand Down
138 changes: 111 additions & 27 deletions gap/oper.gi
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -166,7 +166,13 @@ end);

InstallMethod(DigraphRemoveVertices, "for an immutable digraph and a list",
[IsImmutableDigraph, IsList],
{D, list} -> MakeImmutable(DigraphRemoveVertices(DigraphMutableCopy(D), list)));
function(D, list)
if ForAll(list, IsPosInt) and
not (ForAny(list, x -> x <= DigraphNrVertices(D))) then
Copy link
Collaborator

Choose a reason for hiding this comment

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

If the list has negative integers, then presumably the input digraph should still not be copied?

Instead of testing IsPosInt and then checking <=, would we be better to check ForAny(list, x -> not x in DigraphVertices(D)? Then it's a single clean statement that catches everything.

return D;
fi;
return MakeImmutable(DigraphRemoveVertices(DigraphMutableCopy(D), list));
end);

#############################################################################
# 2. Adding and removing edges
Expand Down Expand Up @@ -223,11 +229,7 @@ InstallMethod(DigraphAddEdges, "for an immutable digraph and a list",
[IsImmutableDigraph, IsList],
{D, edges} -> MakeImmutable(DigraphAddEdges(DigraphMutableCopy(D), edges)));

InstallMethod(DigraphRemoveEdge,
"for a mutable digraph by out-neighbours and two positive integers",
[IsMutableDigraph and IsDigraphByOutNeighboursRep, IsPosInt, IsPosInt],
function(D, src, ran)
local pos;
DIGRAPHS_CheckDigraphRemoveEdge := function(D, src, ran)
if IsMultiDigraph(D) then
ErrorNoReturn("the 1st argument <D> must be a digraph with no multiple ",
"edges,");
Expand All @@ -238,6 +240,14 @@ function(D, src, ran)
ErrorNoReturn("the 3rd argument <ran> must be a vertex of the ",
"digraph <D> that is the 1st argument,");
fi;
end;

InstallMethod(DigraphRemoveEdge,
"for a mutable digraph by out-neighbours and two positive integers",
[IsMutableDigraph and IsDigraphByOutNeighboursRep, IsPosInt, IsPosInt],
function(D, src, ran)
local pos;
DIGRAPHS_CheckDigraphRemoveEdge(D, src, ran);
pos := Position(D!.OutNeighbours[src], ran);
if pos <> fail then
Remove(D!.OutNeighbours[src], pos);
Expand All @@ -249,8 +259,19 @@ end);
InstallMethod(DigraphRemoveEdge,
"for a immutable digraph and two positive integers",
[IsImmutableDigraph, IsPosInt, IsPosInt],
{D, src, ran}
-> MakeImmutable(DigraphRemoveEdge(DigraphMutableCopy(D), src, ran)));
function(D, src, ran)
local pos, DMutable;
DMutable := DigraphMutableCopy(D);
DIGRAPHS_CheckDigraphRemoveEdge(DMutable, src, ran);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Here we create a copy and then check it is valid. Why not check the original graph before copying, so that extra work is not done?

pos := Position(D!.OutNeighbours[src], ran);
if pos <> fail then
Remove(DMutable!.OutNeighbours[src], pos);
RemoveDigraphEdgeLabel(DMutable, src, pos);
return MakeImmutable(DMutable);
else
return D;
fi;
end);

InstallMethod(DigraphRemoveEdge, "for a mutable digraph and a list",
[IsMutableDigraph, IsList],
Expand All @@ -264,7 +285,13 @@ end);
InstallMethod(DigraphRemoveEdge,
"for a immutable digraph and a list",
[IsImmutableDigraph, IsList],
{D, edge} -> MakeImmutable(DigraphRemoveEdge(DigraphMutableCopy(D), edge)));
function(D, edge)
if Length(edge) <> 2 then
ErrorNoReturn("the 2nd argument <edge> must be a list of length 2,");
else
return DigraphRemoveEdge(D, edge[1], edge[2]);
fi;
end);

InstallMethod(DigraphRemoveEdges, "for a digraph and a list",
[IsMutableDigraph, IsList],
Expand All @@ -275,7 +302,32 @@ end);

InstallMethod(DigraphRemoveEdges, "for an immutable digraph and a list",
[IsImmutableDigraph, IsList],
{D, edges} -> MakeImmutable(DigraphRemoveEdges(DigraphMutableCopy(D), edges)));
function(D, edges)
local DMutableRemoved;
DMutableRemoved := DigraphRemoveEdges(DigraphMutableCopy(D), edges);

# If any edges have been removed
if DigraphNrEdges(D) <> DigraphNrEdges(DMutableRemoved) then
Copy link
Collaborator

Choose a reason for hiding this comment

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

Again, could we somehow check this before copying, to avoid the extra work if it's not necessary?

I think you could check all the inputs with IsDigraphEdge rather than comparing numbers after a copy.

return MakeImmutable(DMutableRemoved);
else
return D;
fi;
end);

DIGRAPHS_DigraphReverseEdgeDoOper := function(D, u, v, pos)
Remove(D!.OutNeighbours[u], pos);
Add(D!.OutNeighbours[v], u);
if Length(Positions(D!.OutNeighbours[v], u)) > 1 then
# output is a multidigraph
ClearDigraphEdgeLabels(D);
elif IsBound(D!.edgelabels)
and IsBound(D!.edgelabels[u])
and IsBound(D!.edgelabels[u][pos]) then
SetDigraphEdgeLabel(D, v, u, D!.edgelabels[u][pos]);
RemoveDigraphEdgeLabel(D, u, pos);
fi;
return D;
end;

InstallMethod(DigraphReverseEdge,
"for a mutable digraph by out-neighbours and two positive integers",
Expand All @@ -293,24 +345,28 @@ function(D, u, v)
elif u = v then
return D;
fi;
Remove(D!.OutNeighbours[u], pos);
Add(D!.OutNeighbours[v], u);
if Length(Positions(D!.OutNeighbours[v], u)) > 1 then
# output is a multidigraph
ClearDigraphEdgeLabels(D);
elif IsBound(D!.edgelabels)
and IsBound(D!.edgelabels[u])
and IsBound(D!.edgelabels[u][pos]) then
SetDigraphEdgeLabel(D, v, u, D!.edgelabels[u][pos]);
RemoveDigraphEdgeLabel(D, u, pos);
fi;
return D;
return DIGRAPHS_DigraphReverseEdgeDoOper(D, u, v, pos);
end);

InstallMethod(DigraphReverseEdge,
"for an immutable digraph, positive integer, and positive integer",
[IsImmutableDigraph, IsPosInt, IsPosInt],
{D, u, v} -> MakeImmutable(DigraphReverseEdge(DigraphMutableCopy(D), u, v)));
function(D, u, v)
local pos;
if IsMultiDigraph(D) then
ErrorNoReturn("the 1st argument <D> must be a digraph with no ",
"multiple edges,");
fi;
pos := Position(D!.OutNeighbours[u], v);
if pos = fail then
ErrorNoReturn("there is no edge from ", u, " to ", v,
" in the digraph <D> that is the 1st argument,");
elif u = v then
return D;
fi;
return MakeImmutable(DIGRAPHS_DigraphReverseEdgeDoOper(
DigraphMutableCopy(D), u, v, pos));
end);

InstallMethod(DigraphReverseEdge, "for a mutable digraph and list",
[IsMutableDigraph, IsList],
Expand All @@ -323,7 +379,13 @@ end);

InstallMethod(DigraphReverseEdge, "for an immutable digraph and a list",
[IsImmutableDigraph, IsList],
{D, e} -> MakeImmutable(DigraphReverseEdge(DigraphMutableCopy(D), e)));
function(D, e)
if Length(e) <> 2 then
ErrorNoReturn("the 2nd argument <e> must be a list of length 2,");
else
return DigraphReverseEdge(D, e[1], e[2]);
fi;
end);

InstallMethod(DigraphReverseEdges, "for a mutable digraph and a list",
[IsMutableDigraph, IsList],
Expand All @@ -334,7 +396,23 @@ end);

InstallMethod(DigraphReverseEdges, "for an immutable digraph and a list",
[IsImmutableDigraph, IsList],
{D, E} -> MakeImmutable(DigraphReverseEdges(DigraphMutableCopy(D), E)));
function(D, E)
local changed, DMutable, edge;
changed := false;
DMutable := DigraphMutableCopy(D);
for edge in E do
DigraphReverseEdge(DMutable, edge);
if edge[1] <> edge[2] then # The edge could be reversed
changed := true;
fi;
od;

if changed = true then
return MakeImmutable(DMutable);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Again, could we figure out whether the condition is true without creating a copy first?

else
return D;
fi;
end);

InstallMethod(DigraphClosure,
"for a mutable digraph by out-neighbours and a positive integer",
Expand Down Expand Up @@ -1218,8 +1296,14 @@ end);
InstallMethod(QuotientDigraph,
"for an immutable digraph and a homogeneous list",
[IsImmutableDigraph, IsHomogeneousList],
{D, partition} ->
MakeImmutable(QuotientDigraph(DigraphMutableCopy(D), partition)));
function(D, partition)
if DigraphNrVertices(D) = 0 and IsEmpty(partition) then
return D;
else
return MakeImmutable(QuotientDigraph(DigraphMutableCopy(D), partition));

fi;
end);

#############################################################################
# 6. In and out degrees, neighbours, and edges of vertices
Expand Down
2 changes: 1 addition & 1 deletion tst/standard/oper.tst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -762,7 +762,7 @@ gap> DigraphVertexLabels(D);
gap> gr := CompleteDigraph(4);
<immutable complete digraph with 4 vertices>
gap> gr2 := DigraphRemoveVertices(gr, []);
<immutable digraph with 4 vertices, 12 edges>
<immutable complete digraph with 4 vertices>
gap> gr = gr2;
true
gap> gr2 := DigraphRemoveVertices(gr, [0]);
Expand Down
Loading