Implement simple cycle search Python binding

[GenieTim]

This is the new iteration of #803 after switching to a new branch.

[szhorvat]

@ntamas What kind of interface do you prefer for returning vertices or edges? This function returns both at the moment. In contrast, get_shortest_paths() has an output parameter that controls whether to return one or the other (and could in principle be extended to support returning both).

I don't have a strong opinion, and I'm not experienced enough with Python to know what's the most natural.

[ntamas]

This change is unrelated to the PR but it's a nice addition. File a separate PR with this and I'll be happy to merge.

[szhorvat]

C/igraph needs CMake 3.18 or later.

[szhorvat]

@ntamas Is CMake needed even when building with an external igraph? I thought pkg-config was sufficient then.

[ntamas]

It's not needed for an external igraph but it's needed for the vendored one. There's no way to specify this to Python build backends, though so it's easier to just include the build dependency - it won't hurt to have it there even if the user ends up linking to an external igraph.

[ntamas]

I don't have a strong opinion, and I'm not experienced enough with Python to know what's the most natural.

Well, neither option is really "natural"; I believe that if this was a pure Python package I would have implemented a dataclass with two members, one for the vertices and one for the edges.

The new ctypes-based interface would generate a function that returns a tuple with both output arguments. In the case of the now-current Python interface I would try to keep things consistent with get_shortest_paths() so let's try to do it the same way unless it becomes super complicated.

[GenieTim]

The new ctypes-based interface would generate a function that returns a tuple with both output arguments. In the case of the now-current Python interface I would try to keep things consistent with get_shortest_paths() so let's try to do it the same way unless it becomes super complicated.

My argument against that is the fact, that you have to call the function twice if you want both. Given the high performance impact for large graphs, it seems a bit wasteful to force the user to run it twice.

To me, returning a tuple is pythonic; there are plenty of built-in Python functions that act just like that, for example divmod, zip, enumerate or dict.items.

Correspondingly, if this is an open discussion, I would rather propose to open a PR to change get_shortest_paths() to return a tuple as well. If it's not an open discussion, or you would not want to impose this breaking change upon the users, I will adjust the code to act accordingly.

One possibility: introduce an optional output parameter as requested, but allow a third value, "both", in which case it acts as I propose it to and returns the edges & vertices as a tuple. Then, I would introduce the same for the get_shortest_paths() function and possibly deprecate the output parameter.

[szhorvat]

For context, a new Python interface is in the works: https://github.com/igraph/python-igraph-ctypes Tuples definitely do make sense and as @ntamas said, the new interface will use them.

One possible option for the old interface is to support output = "both", which will return a tuple, without breaking the existing interface.

It is still a question how much effort is worth putting into the old interface given that the new one is coming. Many people do use the old interface, and since at the moment we are very low on resources, it may take a long before it becomes ready. Thus "low resources" becomes and argument both for and against continuing to improve the old interface.

The one to make the final decision in @ntamas

[GenieTim]

A sorry, I was not aware of this new interface and misemphasized the "would", thank you for the context. Then I agree, it's probably best to stick to what we have now. I will draft a new PR soonish.

[ntamas]

Let's try supporting output="both" and then we can return a tuple there. Typing would be a bit more complicated for such a function, but the old interface does not use types anyway so it's not a problem.

[szhorvat]

This is superseded by #806, right?