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.

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

Merged
merged 3 commits into from
Sep 11, 2024

Conversation

MeikeWeiss
Copy link
Contributor

I had the problem that after calling DigraphAllChordlessCycles for a graph the output of DigraphAllSimpleCircuits 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 that SetDigraphVertexLabels(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:

 g:=Digraph([ [ 2, 3, 7 ], [ 1, 4, 8 ], [ 1, 4, 9 ], [ 2, 3, 10 ], [ 6, 7, 9 ], [ 5, 8, 10 ], [ 8, 5, 1 ], [ 7, 6, 2 ], [ 5, 10, 3 ], [ 6, 9, 4 ] ]);

gap/attr.gi Outdated
digraph := DigraphSymmetricClosure(DigraphRemoveLoops(
DigraphRemoveAllMultipleEdges(DigraphMutableCopyIfMutable(D))));

digraph := DigraphCopySameMutability(DigraphSymmetricClosure(
Copy link
Member

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.

Copy link
Member

@james-d-mitchell james-d-mitchell left a 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.

@james-d-mitchell james-d-mitchell added the bugfix A label for PRs that fix a bug label Sep 10, 2024
@MeikeWeiss
Copy link
Contributor Author

Thank you for your comment! It seems to work.

@james-d-mitchell james-d-mitchell merged commit b080f74 into digraphs:main Sep 11, 2024
25 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix A label for PRs that fix a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants