feat: expose voronoi to python #833
Conversation
BeaMarton13
changed the title
|
Try to avoid re-formatting the |
src/_igraph/graphobject.c
Outdated
| igraph_vector_t *lengths_v = 0; | ||
| igraph_vector_t *weights_v = 0; | ||
| igraph_vector_int_t membership_v, generators_v; | ||
| igraph_neimode_t mode = IGRAPH_ALL; |
There was a problem hiding this comment.
I'm leaning towards making IGRAPH_OUT the default. This is explicitly designed to handle directed graphs properly. Opinions @ntamas ?
If this is changed, the documentation should also be changed from "all" to "out".
There was a problem hiding this comment.
There isn't really any consistency in the Python interface w.r.t. the defaults of mode arguments. Let's see what functions.yaml says in the upstream repo and use that default - this will be used in the new Python interface as well.
src/_igraph/graphobject.c
Outdated
| igraph_vector_t *lengths_v = 0; | ||
| igraph_vector_t *weights_v = 0; |
There was a problem hiding this comment.
I know that this codebase frequently uses 0 for null pointers, but let's aim to be clear with newly added code: NULL should be used for pointers, 0 for numbers, and false for Booleans.
| } | ||
|
|
||
| /* Handle radius parameter */ | ||
| if (radius_o != Py_None) { |
There was a problem hiding this comment.
Useigraphmodule_PyObject_to_real_t here, this simplifies the entire logic that you have coded here. I think it does not handle Py_None so you need to handle that separately.
There was a problem hiding this comment.
Pull Request Overview
This pull request exposes a new Voronoi community detection algorithm to Python.
- Adds new test cases in tests/test_decomposition.py to validate the Voronoi clustering implementation.
- Implements the _community_voronoi function in src/igraph/community.py with detailed documentation.
- Updates the Python API and C extension bindings in src/igraph/init.py and src/_igraph/graphobject.c respectively.
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| tests/test_decomposition.py | Introduces tests for the community_voronoi method using multiple graph scenarios. |
| src/igraph/community.py | Implements the Voronoi clustering algorithm with comprehensive documentation. |
| src/igraph/init.py | Exposes the new community_voronoi function through the public API. |
| src/_igraph/graphobject.c | Adds the C extension binding for community_voronoi, handling Python–C argument conversion and result packaging. |
tests/test_decomposition.py
Outdated
| self.assertTrue( | ||
| communities == expected_communities or communities == expected_communities[::-1] # Order might be swapped |
There was a problem hiding this comment.
[nitpick] Consider comparing the communities without relying on order (e.g. by converting both collections to a set of frozensets) to make the test more robust.
| self.assertTrue( | |
| communities == expected_communities or communities == expected_communities[::-1] # Order might be swapped | |
| self.assertEqual( | |
| set(frozenset(c) for c in communities), | |
| set(frozenset(c) for c in expected_communities), |
Copilot uses AI. Check for mistakes.
src/_igraph/graphobject.c
Outdated
|
|
||
| /* Handle weights parameter */ | ||
| if (igraphmodule_attrib_to_vector_t(weights_o, self, &weights_v, ATTRIBUTE_TYPE_EDGE)) { | ||
| if (lengths_v != 0) { |
There was a problem hiding this comment.
For clarity, consider checking against NULL (e.g., 'if (lengths_v != NULL)') when working with pointer values.
| if (lengths_v != 0) { | |
| if (lengths_v != NULL) { |
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
Good point, this would be more readable.
I used Cody for the functions.