-
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
Add Zykov and Christofides Algorithms for Chromatic Number #491
base: main
Are you sure you want to change the base?
Add Zykov and Christofides Algorithms for Chromatic Number #491
Conversation
ee0a544
to
4c77fee
Compare
Style changes from review Duplicate tests already included Add outline for Byskov algorithm Add function to find three colourable subgraphs Moved three colourable processing to before Byskov Added declaration for Byskovs Handled Special case for three vertices and no maximal independent sets Wasn't actually getting the complement of the independent set Create category isDigraphAlgorithm and sub-category isDigraphColouringAlgorithm Will be used to distinguish different colouring algorithms that are available. Add Bound Global objects for Digraph Colouring Algorithms Update original functions to use new method selection Update test for new method selection Forgot to subtract set before getting the induced subgraph Remove from extreme test as it is far too large for these tests Move tests that are too slow Special case was not needed Add loop checks to Lawler and Byskov Algorithms Remove Extreme Tests Add standard tests that will run in a reasonable amount of time Not all chromatic number tests were duplicated, as some would use too much memory or take too long Fix formatting More formatting issues Should be the last one Added Documentation Misc Comments Make label match docs Optimise away the subdigraph use Remove another copy Fix lawler issues Optimise Byskov clarify todo Revert to using induced subdigraph Fix some linting Missed a few Fix Indent Add method selection objects for Zykov Initial algorithm skeleton Fill in the rest Forgot about edge direction Documentation Start adaptation for pruned trees Simplify logic Add tests for Zykov Add filters and method objects for Christofides Initialise variable for Christofides Update comments Initial version of Christofides implemented. Fixed a few syntax errors Fix calculation of MIS Remove debug print Cleanup christofides fix typo Convert to using Blists Need to benchmark to check improvement Fix formatting Missed some trailing whitespace Restart attempt at in place zykov Revert to just copying In place is more hassle than it's worth Remove redundant extra check Use new function for Christofides Fix merge issues Add vertex ordering to Zykov Add Christofides test Optimise clique finder used Fix lint remove duplicate bib Update for new method selection Fix test output Update Docs Remove unused reference Fix spelling mistake
4c77fee
to
101c55d
Compare
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.
Looks pretty reasonable to me.
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.
This generally looks good, mod some comments. If you're able to address the comments, that'd be great
true)) then | ||
# Get adjacency function | ||
adjacent := DigraphAdjacencyFunction(D); | ||
# Sort vertices by degree, so that higher degree vertices are picked first |
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.
This is already implemented in DigraphWelshPowellOrder
maybe you could use that instead of rolling your own?
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.
Thank you for letting me know about DigraphWelshPowellOrder
, I'll make the change to use that instead!
# A contraction of a graph effectively merges two non adjacent vertices | ||
# into a single new vertex with the edges merged. | ||
# We merge y into x, keeping x. | ||
D_contract := DigraphMutableCopy(D); |
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.
Can't you use DigraphContractEdge
to do this step too?
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.
Looking through the manual I can't seem to find any reference to DigraphContractEdge
, but I believe it would be possible to replace the step with DigraphQuotient
by taking the partition the two adjacent vertices in one part, and then the remaining vertices are singleton parts. I need to look into the implementation of DigraphQuotient
, as it seems more general that what is needed here and so could be slower.
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 see now there is an open pull request for DigraphContractEdge
, so I'll use that once the required changes are made.
gap/attr.gi
Outdated
# Choose two non-adjacent vertices x, y | ||
# This is just done by ascending ordering. | ||
found := false; | ||
for x_i in [1 .. nr] do |
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.
This seems like a lot of work to find two non-adjacent vertices, couldn't you just take the last node u
in the list vertices
(which has lowest degree) and take the any value not in OutNeighbours(D, u);
? This has to exist because u
is of minimum degree and D
is not a complete graph.
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.
The reasoning behind the approach is that the recursion will terminate either when it finds a clique the size of the current upper bound or we reach the complete graph. By prioritising high degree vertices, it is more likely we will reach a clique in fewer steps. I'd be happy to run a few experiments though to test if you think it is worth checking.
chrom := Minimum(nr, chrom); | ||
# Leaf nodes are either complete graphs or q-cliques. The chromatic number | ||
# is then the smallest q-clique found. | ||
if not IsCompleteDigraph(D) and IsEmpty(CliquesFinder(D, fail, [], 1, [], |
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.
Could you possibly add a comment to this line about what it is that the CliquesFinder
function is looking for here?
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'll add some additional explanation for the CliqueFinder
call.
gap/attr.gi
Outdated
# Pick u in V \ T such that u is in the fewest maximal independent sets. | ||
u := -1; | ||
min_occurences := infinity; | ||
for i in vertices do |
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.
This seems to have unnecessarily high complexity. Why not loop over everything in v_without_t
one time to find a list whose i-th entry is the number of times it occurs in the an entry of v_without_t
?
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.
That is a more sensible approach, I'll make that change now
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 experimented with both this approach and the current approach, and it seemed to not have a noticeable difference. I've instead simplified the current approach, so that it should be clearer and neater.
1220a16
to
4db0f0c
Compare
Add additional explanation to Zykov Improve vertex picking for Zykov Simplify vertex picking for Christofides
4db0f0c
to
0af7fc2
Compare
@james-d-mitchell I've made the requested changes based on our discussion yesterday, let me know if you have any further comments. |
Add Zykov and Christofides Algorithms for Chromatic Number. This builds on the previous pull request #382 to add two additional algorithms for calculating the Chromatic Number. These use the same method selection as the existing implementations, and so are simply additional options to use.