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

Fix planarity #696

Merged
merged 14 commits into from
Sep 5, 2024
Merged

Fix planarity #696

merged 14 commits into from
Sep 5, 2024

Conversation

Joseph-Edwards
Copy link
Collaborator

This PR addresses #669.

For planarity checking, the edge-addition-planarity-suite code needs its graphs to be antisymmetric. Previously, the antisymmetric digraph was constructed in GAP, and then the desired output was constructed in C. This meant that the C code couldn't reason about the user's input digraph, and hence only a minimal rotation system was returned.

Now, the antisymmetric digraph is constructed in C, and the return list in constructed by checking the existence of edges in the user's original input graph.

@Joseph-Edwards
Copy link
Collaborator Author

For some of the SubdigraphHomeomorphicToK[...] functions, I'm not 100% sure what the best output/documentation should be. At the moment, on input D, these functions return subdigraphs G such that, when directions are ignored, it is Homemorphic to K[...]. I don't know if this is desirable (or even the correct use of homeomorphic). For example:

gap> D := CompleteDigraph(4);
<immutable complete digraph with 4 vertices>
gap> SubdigraphHomeomorphicToK4(D);
[ [ 2, 4, 3 ], [ 3, 4, 1 ], [ 4, 2, 1 ], [ 1, 2, 3 ]

An alternative output could be:

[ [ 2, 4, 3 ], [ 3, 4 ], [ 4 ], [ ] ]

It is a question of whether the output should be thought of as a graph (because direction doesn't matter in the case of planarity) or a digraph (because we are in the digraphs package).

I'm happy to change the behaviour to whatever seems the most appropriate, or to provide some more examples.

gap/planar.gi Outdated Show resolved Hide resolved
@james-d-mitchell james-d-mitchell added the bugfix A label for PRs that fix a bug label Sep 2, 2024
gap/attr.gi Outdated Show resolved Hide resolved
gap/attr.gi Outdated Show resolved Hide resolved
@Joseph-Edwards Joseph-Edwards marked this pull request as draft September 3, 2024 09:56
@Joseph-Edwards Joseph-Edwards marked this pull request as ready for review September 4, 2024 15:58
@Joseph-Edwards
Copy link
Collaborator Author

Provided the tests pass, I think this is good to go @james-d-mitchell

@Joseph-Edwards
Copy link
Collaborator Author

Joseph-Edwards commented Sep 4, 2024

I've just noticed this:

gap> IsPlanarDigraph(HanoiGraph(6));
true
gap> IsPlanarDigraph(DigraphMutableCopy(HanoiGraph(6)));
false

Not sure what's going on, but this probably shouldn't happen. This behaviour exists in the main branch as well. Trying to fix now.

@Joseph-Edwards
Copy link
Collaborator Author

I've just noticed this:

gap> IsPlanarDigraph(HanoiGraph(6));
true
gap> IsPlanarDigraph(DigraphMutableCopy(HanoiGraph(6)));
false

Not sure what's going on, but this probably shouldn't happen. This behaviour exists in the main branch as well. Trying to fix now.

I think this is a problem with HanoiDigraph rather than the planarity stuff (see #698 ).

@james-d-mitchell
Copy link
Member

Hmm that seems fishy, the return value shouldn't depend on whether or not the graph is mutable.

@Joseph-Edwards
Copy link
Collaborator Author

Joseph-Edwards commented Sep 4, 2024

@james-d-mitchell HanoiGraph sets the property IsPlanarDigraph to true. My guess is that this value gets returned when dealing with the immutable one, but the planarity actually gets computed (and returns false) in the mutable case.

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.

One minor tweak and this is good to go

tst/standard/attr.tst Outdated Show resolved Hide resolved
@james-d-mitchell james-d-mitchell merged commit a6da28d into digraphs:main Sep 5, 2024
26 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