Simple Cycle Search

[GenieTim]

This could one day fix #1398, but still requires lots of work.

As of writing this (after first commit), I guess Johnson's algorithm is sketched out, but the results are never returned as I have not yet agreed on what structure I want to use to persist the search state between different calls to the function.
Various other todos are noted in the code.

[codecov]

Codecov Report

Attention: Patch coverage is 94.15205% with 10 lines in your changes missing coverage. Please review.

Project coverage is 84.33%. Comparing base (f5d79f2) to head (c6820ea).
Report is 1 commits behind head on master.

Files with missing lines	Patch %	Lines
src/cycles/simple_cycles.c	94.15%	10 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2181      +/-   ##
==========================================
+ Coverage   84.31%   84.33%   +0.02%     
==========================================
  Files         380      381       +1     
  Lines       63015    63186     +171     
  Branches    12293    12322      +29     
==========================================
+ Hits        53130    53291     +161     
- Misses       9885     9895      +10     

Files with missing lines	Coverage Δ
src/cycles/simple_cycles.c	94.15% <94.15%> (ø)

---

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f5d79f2...c6820ea. Read the comment docs.

[szhorvat]

Great to see such quick progress!

Small note: Be sure to work on the latest develop branch. Can you rebase your changes onto the latest version?

igraph 0.10 will be released within days, at which point the develop branch will be merged into master. Then we can also retarget this PR for master. We do plan to add new functions in 0.10.x releases, but not change any existing ones.

Because of the workload with the release, we won't be able to look at this PR for a while. We can discuss the issue with persisting the state when the dust settles a bit.

[szhorvat]

develop is already merged back into master, so you should rebase onto master now.

[ntamas]

There are quite a few unrelated changes in this PR that should ideally be reverted in order to make it easier to review later. These unrelated changes can roughly be grouped as follows:

• Code reformatting probably done by your IDE, like putting braces on separate lines. These should simply be reverted.
• Replacing () with (void) in C++ files or headers that are solely included in C++ files; these are not needed and should also be reverted.
• Changes in vendor/f2c; here you also replaced () with (void) but in places that are not used (they are guarded by an #ifdef).

Can you revert these so we can see better what the relevant changes are?

[GenieTim]

Yes, sure, thanks for the feedback, will do. Currently, there is lots other things wrong in my code that I might focus on first; I suppose you will want to squash merge this PR in the end anyway, if at all

[ntamas]

Also, what will you need igraph_vector_pop_front() for? If you want to be able to pop items off the front or the back of a vector, you can probably use igraph_dqueue_t; igraph_dqueue_pop() will pop from the front and igraph_dqueue_pop_back() will pop from the back.

[GenieTim]

If you want to be able to pop items off the front or the back of a vector, you can probably use igraph_dqueue_t; igraph_dqueue_pop() will pop from the front and igraph_dqueue_pop_back() will pop from the back.

Yes, I actually have a comment where I use the igraph_vector_pop_front reminding me to replace it with dqueue in the end. It was just easier to start with the adjlist which has already all the dimensionality handled for me for now.

[szhorvat]

@ntamas Let me know if you have any comments before I merge this.

@GenieTim Can you please also have a look, in case I missed something?

I would like to merge this before the next release.

One issue is that the function gets slow on large graphs with few cycles, such as a very large cycle graph. We can try to improve on this later. On small graphs with a huge number of cycles (which are likely to be a common use case) the performance is good.

I made the functions for searching from a single vertex private for now, as I'm not sure what the best way is to expose these, and I don't want to delay the PR further. One such unused function is commented.

Commented debugging code is left in for now.

[szhorvat]

Added a mode parameter and made sure that non-public functions have _i_ in the name. Removed the unused function. Tested with function wit libFuzzer.

[szhorvat]

A big thank you for your hard work, and patience on this @GenieTim! If all goes well, it will be released later this week.

I'd love to have this in the Python interface, to actually get it out to users. If you have time to help with that, let me know, and I ca give you the basic steps. https://github.com/igraph/python-igraph/ Exposing igraph_simple_cycles() should be easy, igraph_simple_cycles_callback() a bit more work, but that can come later. I'll look into exposing it in R and finishing the interface (which I used for testing) in Mathematica.

We can continue making improvements, and expose more functions which are now internal, at a later stage. It might also be good to look into the performance on large cycles (I guess this relates to not running the search from all vertices).

[GenieTim]

Well, thank you too.

Progress with the Python binding will be handled in igraph/python-igraph#803