WIP: Add an is_chordal algorithm#434
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #434 +/- ##
==========================================
- Coverage 97.29% 97.28% -0.01%
==========================================
Files 123 127 +4
Lines 7421 7699 +278
==========================================
+ Hits 7220 7490 +270
- Misses 201 209 +8 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Do you by any change know where I could find a public accessible version of that paper? My usual methods did not help. |
|
The paper is attached here, courtesy of the UMass library. |
Wonderful, @Krastanov, thanks! |
|
@simonschoelly, just wanted to let you know I certainly haven't abandoned this PR. I was busy with work this past week and I'm at a conference this weekend, but I'll get back to it shortly. I've looked over your comments and it shouldn't be many more changes at all anyway, hehe. |
|
@simonschoelly, just changed the input type from |
|
Wait—I just remembered your comment about induced subgraphs. I'll think about that and get back to you I was largely just porting over networkx's implementation, which does do this, but it probably really isn't optimal. |
Don't worry - we are quite slow with reviewing PRs here :D |
|
@simonschoelly @Krastanov I've fixed the issue regarding unnecessary allocations with |
|
Hi @simonschoelly, just following up. Is everything good enough for me to conclude my work on the actual logic and start on the docstring and unit tests? 🙂 |
|
What do you think, @Krastanov? 🙂 |
|
Thanks, @Luis-Varona ! No need to wait for anything else to be finished before adding tests on my end. I actually frequently refuse to review PRs before the tests are added, as I judge quality first by the tests and documentation. I am adding some quality of life improvements to IGraphs.jl that should make it easy to test against their implementation as well. I will post it here shortly. |
|
Just to clarify: while I might have a particular style of review, that style is not shared by everyone, and please do not hesitate to request me approaching this differently. |
|
IGraphs.jl just got a new release in which you can use the following in order to test your implementation against the ones in the igraph C library https://igraph.org/c/html/latest/igraph-Structural.html#igraph_is_chordal I frequently use random sampling over appropriate ensembles as a way to test that two implementations give the same result. This can be useful here as well. It can also be fun to compare performance (just make sure to not count the time to convert the graph from one format to the other). |
|
There is also https://github.com/wangjie212/ChordalGraph |
|
You resolved that conversation but might have overlooked what I wrote there about using the |
|
Thanks for the info, @Krastanov!
So I did, @simonschoelly. Sorry—I must've overlooked that. I expect that sort of fix shouldn't take long, anyway, so I'll try to take a crack at it later this week. I suppose regardless of the internal workings of our version of Edit: Certainly, benchmarking our implementation against both of the above would be useful, but I'm thinking I'll leave that to the |
|
Yes, no need to test against multiple other libraries. Your own correctness tests are probably most important -- testing against another library with a bunch of random samples just provides a bit more certainty that we are not missing anything. |
|
Hi @Krastanov and @simonschoelly, sorry for the long silence—I plan to return to this issue sometime next week. IIRC, all that's left is just a test suite, and maybe a docs fix. 🙂 |
|
Thanks @Luis-Varona |
|
@Luis-Varona @Krastanov @simonschoelly I have an extremely performant implementation of this function here, if that is of any interest. In particular, the MCS algorithm is best implemented using a bucket queue: see here. |
|
@samuelsonric , thanks for the heads up! It would be great to have access to both implementations from Graphs.jl -- this also makes testing much easier as we can just do random tests against each other. If you or anyone else has the bandwidth, my suggestion would be to do
As for 
|
|
I am happy to do this. I can probably implement |
|
Thanks, Richard! |
Benchmark Results (Julia v1)Time benchmarks
Memory benchmarks
|
Luis-Varona
force-pushed
the
431-is_chordal-algorithm
branch
from
d2eb7cc to
fdcc770
Compare
We implement Tarjan and Yannakakis (1984)'s MCS algorithm, taking inspiration from the existing NetworkX implementation. Everything is done except for example doctests in src/chordality.jl and unit tests in test/chordality.jl. (This PR is part of a new suite of algorithms outlined in issue JuliaGraphs#431.)
Originally, we restricted the input type for is_chordal to AbstractSimpleGraph to rule out parallel edges, but some other graph types we would like to support (such as SimpleWeightedGraph) are of the more general AbstractGraph type. It turns out the current AbstractGraph interface aleady does not support parallel edges, so there is no worry here--it is already stated in the Implementation Notes anyway that is_chordal should not take in graphs with parallel edges as input.
Originally mirroring the networkx implementation, we created a new AbstractGraph object every time we tested subsequent neighbors in the potential PEO with the induced_subgraph function. This commit makes our implementation more performant by simply taking the vertex (sub)set and checking to see if all pairs are adjacent via iteration.
Additionally, we change inconsistent naming in certain parts ('node' vs 'vertex'), changing everything to 'vertex' where relevant.
Luis-Varona
force-pushed
the
431-is_chordal-algorithm
branch
from
fdcc770 to
4e46aab
Compare
|
Hi @simonschoelly and @Krastanov - I'm really sorry for the long absence. I've just gotten back to work on this PR. (Because it kept getting messy from me merging back in from @simonschoelly, I incorporated (in the latest commit) your trait dispatch suggestions; hopefully it aligns with what you had in mind. I've also added docstring examples, @Krastanov. The next step, I suppose, is to update the test suite, and add to the documentation and changelog. (Both of those things I am to do within the day.) As for @samuelsonric's performant implementation, I'll certainly take a look. |
We create docs/src/algorithms/chordality.md, adding it to the Algorithms API section of docs/make.jl.
|
Thanks, @Luis-Varona . Indeed the main next step would be tests, including correctness tests against other libraries. No worries about the delay, we probably would not have had an active volunteer to help with review at that time anyway. |
|
Got it @Krastanov! Do you suggest including dispatch (re: @samuelsonric's implementation) in this PR or in a future one after the basic functionality has been finalized here? |
|
Your PR is the first one to introduce an implementation of |
We add IGraphs as a test dependency (to the nested Project.toml in the test folder, and import it as well in runtests) for our soon-to-be-completed chordality test suite.
The GenericGraph type used in the unit tests returns a generator object when neighbors is called on it. Originally, we computed the subsequent_neighbors variable via intersect(neighbors(g, v), numbered), but intersect could not infer the element type of a generator object and fell back to Vector{Any}. This caused a MethodError when passing the result to _induces_clique, so we replaced the intersect call with a filter call.
Short-circuit one-liners (e.g., 'A || do B') are expanded into 'if' blocks (similarly, 'if !A; do B; end'), and the cardinality closure in '_max_cardinality_vertex' is inlined as a lambda function.
Luis-Varona
force-pushed
the
431-is_chordal-algorithm
branch
from
6708985 to
10c885b
Compare
|
@Krastanov, I've finished the test suite locally and an just adding some comments for clarity before pushing. I'll probably finish it either before sleeping or sometime tomorrow. 🙂 (And of course, I'll update the CHANGELOG as well once I'm done with that.) |
4 participants
We implement Tarjan and Yannakakis (1984)'s MCS algorithm, taking inspiration from the existing NetworkX implementation. Everything is done except for example doctests in
src/chordality.jland unit tests intest/chordality.jl. (This PR is part of a new suite of algorithms outlined in issue #431.)