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

Jump to bottom

Improve DigraphColouring #144

Merged
merged 1 commit into from
Nov 28, 2018
Merged

Improve DigraphColouring #144

merged 1 commit into from
Nov 28, 2018

Conversation

james-d-mitchell
Copy link
Member

@james-d-mitchell james-d-mitchell commented Nov 15, 2018 •
edited
Loading

This PR implements the Welsh-Powell algorithm for digraph colouring. This implementation is almost always as good as the previous one (in terms of the number of colours required), and is faster (approx.
4 times faster on DigraphRemoveLoops(RandomDigraph(10000, 0.1))).

@james-d-mitchell james-d-mitchell added enhancement A label for PRs that provide enhancements. 0.13 labels Nov 15, 2018
@wilfwilson
Copy link
Collaborator

Nice. Do you think it's faster because you implemented it better, or because it's an inherently faster algorithm?

I'll be happy to merge it, but I do have one request: could you update the documentation for DigraphColouring to mention the name of the algorithm? Currently it says quite generically that it uses a greedy algorithm, but now it would be nice to be more specific now that we can be.

@james-d-mitchell
Copy link
Member Author

Good idea I’ll do this tomorrow

@james-d-mitchell
Copy link
Member Author

@wilfwilson on second reflection, I wonder if it might not be a good idea to have DigraphGreedyColouring, DigraphWelshPowellColouring and so on. Given that we have an implementation of the standard greedy algorithm, it seems a shame to throw it away, given that there are some cases when it gives a better colouring than the new version. What do you think?

@wilfwilson
Copy link
Collaborator

I support this. The remaining question would be what do we do with DigraphColouring? Do we remove it? It does it default to one of them? Or will it use a heuristic to choose the appropriate algorithm?

@james-d-mitchell
Copy link
Member Author

@wilfwilson I'd propose to keep DigraphColouring, and use the version of the algorithm which gives the "best" colouring in the most cases. So, currently, that'd be the Welsh-Powell.

@wilfwilson
Copy link
Collaborator

@james-d-mitchell Sounds good!

@wilfwilson wilfwilson added 0.14 and removed 0.13 labels Nov 23, 2018
@wilfwilson wilfwilson changed the base branch from stable-0.13 to stable-0.14 November 23, 2018 14:01
@james-d-mitchell
Copy link
Member Author

@wilfwilson I've finished this now. I decided to go in a slightly different direction. I changed to make DigraphGreedyColouring for a digraph and an ordering of its vertices, the main function. This way we can get the old behaviour by doing:

DigraphGreedyColouring(D, [1 .. DigraphNrVertices(D)]);

and the new behaviour by doing:

DigraphGreedyColouring(D); # or
DigraphGreedyColouring(D,. DigraphWelshPowellOrder); # or
DigraphGreedyColouring(D,. DigraphWelshPowellOrder(D)); # or

Let me know if spot any issues!

@james-d-mitchell
Copy link
Member Author

Nuts, thought I did everything

@wilfwilson wilfwilson added 0.15 and removed 0.14 labels Nov 24, 2018
@wilfwilson wilfwilson changed the base branch from stable-0.14 to master November 24, 2018 16:07
wilfwilson
wilfwilson reviewed Nov 24, 2018
View reviewed changes
Copy link
Collaborator

@wilfwilson wilfwilson left a comment

Choose a reason for hiding this comment

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

Hi @james-d-mitchell, I like this, I spotted a typo and it would be really nice if you could get test code coverage for the two lines that I pointed out.

I've changed this PR to be against master rather than stable-0.14, but I imagine the next release will be 0.15, and probably fairly soon (especially if the shortest path PR is updated and merged).

gap/grahom.gi
@@ -202,6 +202,15 @@ function(digraph, n)
"the second argument <n> must be a non-negative integer,");
fi;

if HasDigraphGreedyColouring(digraph) then
if DigraphGreedyColouring(digraph) = fail then
return fail;
Copy link
Collaborator

Choose a reason for hiding this comment

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

This line is not tested by grahom.tst

gap/grahom.gi
SubtractBlist(available, BlistList([1 .. n], out[v]));
SubtractBlist(available, BlistList([1 .. n], inn[v]));
if available = empty then
break;
Copy link
Collaborator

Choose a reason for hiding this comment

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

This line is not tested by grahom.tst

doc/grahom.xml Outdated
colouring.
<P/>

If <A>digraph</A> is a digraphs and <A>order</A> is a dense list consisting
Copy link
Collaborator

Choose a reason for hiding this comment

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

Typo: is a digraphs -> is a digraph

tst/standard/attr.tst
@@ -1301,16 +1301,16 @@ gap> gr := DigraphFromDigraph6String(Concatenation(
<digraph with 45 vertices, 180 edges>
gap> ChromaticNumber(gr);
3
gap> DigraphColoring(gr, 3);
gap> DigraphColouring(gr, 3);
Copy link
Collaborator

@wilfwilson wilfwilson Nov 24, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Perhaps these next three tests (using Digraph(Greedy)Colouring) should be moved to grahom.tst, if they're executing the code from grahom.gi.

Copy link
Member Author

Choose a reason for hiding this comment

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

I made all of the changes you suggest @wilfwilson except this one. It looks like the tests here are designed to double check what ChromaticNumber is doing, so I left them where they are.

@james-d-mitchell
grahom: improve DigraphColouring
30e3fb0 
This commit implements the Welsh-Powell algorithm for digraph colouring.
This implementation is almost always as good as the previous one
(in terms of the number of colours required), and is faster (approx.
4 times faster on DigraphRemoveLoops(RandomDigraph(10000, 0.1))).
@james-d-mitchell james-d-mitchell added the do not merge A label for PRs that should not be merged for whatever reason. label Nov 27, 2018
@james-d-mitchell
Copy link
Member Author

I just thought of something else to add to this, so please don't merge it yet.

@james-d-mitchell
Copy link
Member Author

On second thoughts, I have nothing further to add. Merge if happy @wilfwilson

@james-d-mitchell james-d-mitchell removed the do not merge A label for PRs that should not be merged for whatever reason. label Nov 28, 2018
@wilfwilson wilfwilson merged commit 9b520d7 into digraphs:master Nov 28, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wilfwilson wilfwilson wilfwilson left review comments

Assignees
No one assigned
Labels
enhancement A label for PRs that provide enhancements.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@james-d-mitchell @wilfwilson