Behaviour of DigraphRemoveEdge/DigraphRemoveEdges is inconsistent #260 #723
Conversation
- Edited functions for DigraphRemoveEdge, DigraphRemoveEdges, DigraphReverseEdge, DigraphReverseEdges, DigraphRemoveVertices and QuotientGraph - Checks added to see if any work needs to be done before making a copy
|
|
|
@saffronmciver if you pull from main and then rebase your branch then hopefully the lint job will pass |
gap/oper.gi
Outdated
| if ForAll(list, IsPosInt) and | ||
| not (ForAny(list, x -> x <= DigraphNrVertices(D))) then |
There was a problem hiding this comment.
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.
gap/oper.gi
Outdated
| DMutable := DigraphMutableCopy(D); | ||
| DIGRAPHS_CheckDigraphRemoveEdge(DMutable, src, ran); |
There was a problem hiding this comment.
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?
gap/oper.gi
Outdated
| # If any edges have been removed | ||
| if DigraphNrEdges(D) <> DigraphNrEdges(DMutableRemoved) then |
There was a problem hiding this comment.
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.
gap/oper.gi
Outdated
| 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); |
There was a problem hiding this comment.
Again, could we figure out whether the condition is true without creating a copy first?
|
I'm not sure because I haven't looked at the pr in detail but isn't this a backwards incompatible change? I.e it's not impossible that there's code in existence that relies on a copy being returned even if no changes are made? If so then we should probably not merge this into main just now, but maybe include stable-2.0 branch instead? |
|
@james-d-mitchell: that's correct. This PR even involves changing the documentation to reflect the new behaviour, so it's a breaking change. Do we have a branch for 2.0 yet? |
DigraphRemoveEdge,DigraphRemoveEdges,DigraphReverseEdge,DigraphReverseEdges,DigraphRemoveVerticesandQuotientGraph.