-
Notifications
You must be signed in to change notification settings - Fork 47
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.
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
Added copy in DigraphAllChordlessCycles #702
Conversation
gap/attr.gi
Outdated
digraph := DigraphSymmetricClosure(DigraphRemoveLoops( | ||
DigraphRemoveAllMultipleEdges(DigraphMutableCopyIfMutable(D)))); | ||
|
||
digraph := DigraphCopySameMutability(DigraphSymmetricClosure( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure this resolves the issue, if D
is mutable, then DigraphRemoveAllMultipleEdges(D)
changes D
in-place (I think), same with DigraphRemoveLoops
and DigraphSymmetricClosure
. So when D
is mutable we copy it once here.
If D
is immutable, then each of these 3 functions copies D
, and so too does DigraphCopySameMutability
so we make 4 copies of D
.
If you put the DigraphCopySameMutability
as the inner most function call, then I think would work, but would also still be suboptimal (in terms of the number of copies).
Can't we just instead do
digraph := DigraphMutableCopy(D); # 1 copy
# the next line changes digraph in-place and does not touch the original input D
# also there are no copies.
DigraphSymmetricClosure(DigraphRemoveLoops(DigraphRemoveAllMultipleEdges(digraph)));
MakeImmutable(digraph); # make digraph immutable, o/w it can't store attributes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR @MeikeWeiss, please consider my comment, and let me know if that also works. If it does, then please add your previously failing example as a test case too.
Thank you for your comment! It seems to work. |
I had the problem that after calling
DigraphAllChordlessCycles
for a graph the output ofDigraphAllSimpleCircuits
for this graph was wrong. The output included a list of nodes that did not define a valid cycle. In my opinion the problem was thatSetDigraphVertexLabels(digraph, Reversed(DigraphDegeneracyOrdering(digraph)))
changed the given digraph. So now I added a call so that we only consider a copy of the input graph.This solved my problem, but I'm not sure this is the best way.
The graph I was having problems with was the following: