Add Tarjan and Lengauer's (almost) linear time dominators algorithm

[james-d-mitchell]

This work in progress

[james-d-mitchell]

@marinaanagno @samanthaharper @finnbuck

[james-d-mitchell]

The following things still need to be done (hoops to be jumped through):

• remove the quadratic time version of Dominators. This could be put into the test file as a stand alone function, for testing purposes.
• rename Dominators2 to Dominators and update the documentation etc. Also ensure that Dominators2 is a proper GAP method (like Dominators currently is).
• make DominatorTree a proper GAP operation (like Dominators currently is).
• add documentation for DominatorTree
• add tests for DominatorTree

Also we might consider:

• reenabling the lastlinked optimisation
• making buckets flat again.

[codecov]

Codecov Report

Merging #336 (996aeca) into master (10d1ee8) will increase coverage by 0.04%.
The diff coverage is 95.04%.

@@            Coverage Diff             @@
##           master     #336      +/-   ##
==========================================
+ Coverage   96.96%   97.01%   +0.04%     
==========================================
  Files          45       45              
  Lines       12542    12881     +339     
==========================================
+ Hits        12161    12496     +335     
- Misses        381      385       +4     

Impacted Files	Coverage Δ
gap/oper.gi	98.37% <94.95%> (-0.15%)	⬇️
gap/oper.gd	100.00% <100.00%> (ø)
src/bitarray.h	100.00% <0.00%> (ø)
src/conditions.h	100.00% <0.00%> (ø)
src/homos-graphs.h	100.00% <0.00%> (ø)
src/homos.c	99.14% <0.00%> (+0.11%)	⬆️
src/planar.c	94.11% <0.00%> (+0.17%)	⬆️
src/cliques.c	99.30% <0.00%> (+0.21%)	⬆️
src/bitarray.c	95.23% <0.00%> (+0.23%)	⬆️
src/homos-graphs.c	98.93% <0.00%> (+0.42%)	⬆️
... and 1 more

[james-d-mitchell]

There are still a couple of things to do in this PR, so no need for review just yet.

[marinaanagno]

Thanks @james-d-mitchell! Let me know if there's anything I could do!

[james-d-mitchell]

@marinaanagno when you have a moment, can you please review this PR? Once you've done that I'll squash the commits into one again (I left them separate for the time being so that you can see what I did today), and add you and Sam as co-authors for the commit.

[marinaanagno]

The code seems to agree with the theory stated in the paper and seems to be giving right results. Since this is a project I've been involved though and since I am a newcomer a quick look from another pair of eyes would be great! My only comment is on the complexity, but again it is very possible that I might be missing something.

[marinaanagno]

Everything looks good, I only suggested a change in the phrasing in the complexity part of the documentation

[james-d-mitchell]

Thanks @marinaanagno much appreciated. I wonder if the wording about the complexity in the manual entry for DominatorTree should be adjusted in the same way you suggested for Dominators?

[marinaanagno]

@james-d-mitchell you're absolutely right! Sorry I didn't annotate that!

[marinaanagno]

Sorry for the delay, it occurred to me after I had a look. Compress is a recursive function and I'm afraid we've said that we have to make it non-recursive before merging.

[james-d-mitchell]

Sorry for the delay, it occurred to me after I had a look. Compress is a recursive function and I'm afraid we've said that we have to make it non-recursive before merging.

Good point! I’ll have to fix this, maybe not today though 😎

[marinaanagno]

Sorry for the delay, it occurred to me after I had a look. Compress is a recursive function and I'm afraid we've said that we have to make it non-recursive before merging.

After discussing this issue with @james-d-mitchell we figured out that compress will only run log2 of the number of vertices and no digraph that can be calculated using GAP will hit the recursion limit for compress

[wilfwilson]

Something that reviewers should have in mind while reviewing: what should the return value be?

[wilfwilson]

Thanks @marinaanagno and @samanthaharper for your work on this! I've looked at it for quite a while, including reading parts of the paper (although I don't have the time to read the whole thing).

I see that you've followed the pseudocode of the paper quite closely, so I'm pretty much happy to believe that you've done that correctly (having checked a few bits myself) 🙂. The number of tests give me confidence that you have.

My one slight concern about the code is the following. At points, the paper defines things in terms of the vertices less than another vertex (i.e. 5 < 7). Seeing as you're most familiar with the code, I would just like you to convince yourselves that you are not relying on either the list of out-neighbours or the list of in-neighbours of a vertex being sorted. Because they aren't guaranteed to be sorted (although in-neighbours usually are sorted into ascending order).

Some quick testing indicates this isn't an issue, but I'd appreciate it if you could think about it.

I've 'requested changes' for my suggestions on the documentation.

As for the question 'what should the function return' which @james-d-mitchell brought up on the call today, I now understand that to mean 'should DominatorsTree exist as a user-facing function'? And I don't particularly mind about this either way. But if you keep it, I'm happy for it to return a record as it currently does (just please document what those things mean, mathematically 🙂 ).

[marinaanagno]

Thanks @marinaanagno and @samanthaharper for your work on this! I've looked at it for quite a while, including reading parts of the paper (although I don't have the time to read the whole thing).

I see that you've followed the pseudocode of the paper quite closely, so I'm pretty much happy to believe that you've done that correctly (having checked a few bits myself) 🙂. The number of tests give me confidence that you have.

My one slight concern about the code is the following. At points, the paper defines things in terms of the vertices less than another vertex (i.e. 5 < 7). Seeing as you're most familiar with the code, I would just like you to convince yourselves that you are not relying on either the list of out-neighbours or the list of in-neighbours of a vertex being sorted. Because they aren't guaranteed to be sorted (although in-neighbours usually are sorted into ascending order).

Some quick testing indicates this isn't an issue, but I'd appreciate it if you could think about it.

I've 'requested changed' for my suggestions on the documentation.

As for the question 'what should the function return' which @james-d-mitchell brought up on the call today, I now understand that to mean 'should DominatorsTree exist as a user-facing function'? And I don't particularly mind about this either way. But if you keep it, I'm happy for it to return a record as it currently does (just please document what those things mean, mathematically 🙂 ).

Thank you so much for the review @wilfwilson, I will look carefully at all the changes you suggested probably tomorrow :)

[james-d-mitchell]

@wilfwilson @marinaanagno @samanthaharper I think this is good to go, except possibly: I'm not sure if we ever arrived at a decision about whether or not a vertex should dominate itself, any thoughts?

[marinaanagno]

@james-d-mitchell thanks for this! According to the theory a vertex is not dominated by itself and if there's no reason to include it in the list of dominators, I would suggest we make the algorithm agree with the theory. :)

[wilfwilson]

That was my feeling too.

[james-d-mitchell]

Thanks @marinaanagno and @wilfwilson I've updated the PR to not include a vertex as a dominator of itself (I think). The only other thing that I think was pending was whether or not DominatorTree should be user facing. I think it should because it's really the fundamental thing computed by the algorithm, and Dominators is just a useful additional feature. I've updated DominatorTree to only return the preorder and the immediate dominators, both of which are used by the current Dominators method.

[marinaanagno]

@james-d-mitchell looks good to me! As far as DominatorTree is concerned, as a user working on this project, I almost always use DominatorTree, because it gives me much more information, such as preorder values, semidominators (which is now removed) etc. Unless there is any particular reason to prevent us from making it user facing, and since documentation for it already exists, I don't see any reason to not have it available for the user to use. :) Also, semidominators was a pretty important notion for this algorithm and it was very useful to have it as part of the output in DominatorTree and maybe keeping it in the record would be an option, however I'm not really sure whether it is commonly used as a concept, so maybe there's indeed no reason to keep it in the record. :)

[wilfwilson]

Nice! I've just updated this with the master branch; I'll merge if the CI passes again (which it should!).

[marinaanagno]

Thanks so much @wilfwilson ! :)