Feature/latest igraph #564

SJulianS · 2024-05-08T13:48:01Z

revamped graph_algorithm plugin for easier integration of new algorithms and better compatibility with pybind

szhorvat

I left a few comments which I hope will be helpful.

I realize that it is frustrating to do all this memory management manually. We have a project to produce a thin C++ wrapper library that provides RAII types to make this process much simpler, but due to lack of resources, it'll be (sadly) a long time before it's done. You are welcome to contribute to it or borrow it if you like. At this point there's a proof of concept here: https://github.com/igraph/igraph-cpp It is borrowed from what I used in the Mathematica interface of igraph at https://github.com/szhorvat/IGraphM/blob/master/IGraphM/LibraryResources/Source/IGCommon.h If you do use it, copy the code into your project, and don't count on any stability on our side (yet).

szhorvat · 2024-05-08T15:15:08Z

deps/igraph-0.10.x/CMakeLists.txt

@@ -8,15 +8,20 @@ set (IGRAPH_INCLUDES  "${IGRAPH_INSTALL}/include")
 set (IGRAPH_LIB       "${IGRAPH_INSTALL}/lib/libigraph${CMAKE_SHARED_LIBRARY_SUFFIX}")
 set (HAVE_IGRAPH TRUE)

-message("-- IGRAPH version 0.9.10 : download and build libigraph${CMAKE_SHARED_LIBRARY_SUFFIX} in ${IGRAPH_BASE}")
+message("-- IGRAPH version 0.10.11 : download and build libigraph${CMAKE_SHARED_LIBRARY_SUFFIX} in ${IGRAPH_BASE}")


FYI 0.10.12 is available.

szhorvat · 2024-05-08T15:16:51Z

plugins/graph_algorithm/include/graph_algorithm/algorithms/neighborhood.h

+#include "hal_core/defines.h"
+#include "hal_core/utilities/result.h"
+
+#include <igraph/igraph.h>


Normally this is #include <igraph.h>, and -Iprefix/include/igraph is used instead of -Iprefix/include.

The CMake and pkgconfig files that igraph installs are all set up to provide these include paths.

If possible I'd also rather use find_package() and rely on CMake to have include path set. I guess this is what you suggested. The problem is that this mechanism (to my understanding which is very poor) works only for packages already installed. Our goal however is to build the igraph library together with HAL so we can incorporate the latest version. Therefore the files are not in place yet when we run configuration with CMake.

I guess this is what you suggested.

Yes, indeed.

Our goal however is to build the igraph library together with HAL so we can incorporate the latest version.

Can this be made to work by adding igraph as a submodule? #include <igraph.h> should still work fine in this case. See the cmake-project-2 example here: https://github.com/igraph/igraph-example

Now I have to say that I am far from a CMake expert. igraph's build system is mostly maintained by someone else. So I may be missing something.

But if it's reasonably easy, we're willing to adapt on the igraph side to make it easier to vendor igraph sources in other libraries. I believe this should work with the submodule approach already, but if there are issues, let me know.

FYI I just re-tested the example project I suggested you use and it's not working on my machine. I am not sure why, perhaps newer CMake works differently? We'll take a look and try to fix it.

Actually it's all fine. The submodule in my repo was in an inconsistent state. This approach should be usable. Let me know if there are problems.

Thanks for testing @szhorvat ! We will look into the option to have igraph added as submodule eventually. Some time ago people were reluctant to use submodule since it came with very little control over the installed version and when to upgrade (or avoid upgrade due to API break) the external package - but things might have evolved since. Right now I am very happy that we finally -with your help- managed to link HAL with the latest igraph version. So I'll go ahead and merge the PR by now.

Note that it doesn't need to be a git submodule: things should work for as long as you can get the igraph sources in a directory and use add_subdirectory() in CMake.

That is what we do
here

szhorvat · 2024-05-08T15:20:23Z

plugins/graph_algorithm/src/algorithms/components.cpp

+            igraph_t* igr = graph->get_graph();
+
+            // run scc
+            err = igraph_clusters(igr, &membership, &csize, &number_of_clusters, strong ? IGRAPH_STRONG : IGRAPH_WEAK);


You are using a deprecated function. Your compiler should be showing a warning for these. The documentation also indicates which functions are deprecated and what their replacement is. igraph 1.0 will have igraph_clusters() removed.

fixed, sorry this was a left over from the only piece of code I just copied from the old version...

szhorvat · 2024-05-08T15:20:59Z

plugins/graph_algorithm/src/algorithms/components.cpp

+            if (err != IGRAPH_SUCCESS)
+            {
+                igraph_vector_int_destroy(&membership);
+                return ERR(igraph_strerror(err));
+            }


Suggested change

if (err != IGRAPH_SUCCESS)

{

igraph_vector_int_destroy(&membership);

return ERR(igraph_strerror(err));

}

if (err != IGRAPH_SUCCESS)

{

return ERR(igraph_strerror(err));

}

Do not destroy igraph data structures whose initiailzation failed. They have never been created, so they don't need to be destroyed. We do not ensure that trying to do so won't cause problems (such as a crash).

fixed (as are all the other things below as well). Thanks for taking the time and reviewing the code :D Also, thank you for maintaining igraph!

szhorvat · 2024-05-08T15:21:24Z

plugins/graph_algorithm/src/algorithms/components.cpp

+            err = igraph_vector_int_init(&csize, 0);
+            if (err != IGRAPH_SUCCESS)
+            {
+                igraph_vector_int_destroy(&membership);
+                igraph_vector_int_destroy(&csize);
+                return ERR(igraph_strerror(err));
+            }


Suggested change

err = igraph_vector_int_init(&csize, 0);

if (err != IGRAPH_SUCCESS)

{

igraph_vector_int_destroy(&membership);

igraph_vector_int_destroy(&csize);

return ERR(igraph_strerror(err));

}

err = igraph_vector_int_init(&csize, 0);

if (err != IGRAPH_SUCCESS)

{

igraph_vector_int_destroy(&membership);

return ERR(igraph_strerror(err));

}

Same comment about destroying data structure whose initialization has failed (here and many other places).

szhorvat · 2024-05-08T15:23:01Z

plugins/graph_algorithm/src/algorithms/components.cpp

+            }
+
+            // map back to HAL structures
+            u32 num_vertices = (u32)igraph_vcount(igr);


Nothing wrong with this line, I just wanted to point out that igraph uses igraph_integer_t for all integers. This is a signed type. Its size is configurable at build time to be 32- or 64-bit, and by default it follows the word size of the machine.

szhorvat · 2024-05-08T15:26:26Z

plugins/graph_algorithm/src/algorithms/neighborhood.cpp

+            igraph_vs_t v_sel;
+            if (auto res = igraph_vs_vector(&v_sel, start_gates); res != IGRAPH_SUCCESS)
+            {
+                return ERR(igraph_strerror(res));
+            }


Here's a tip: many of the vs functions have an "immediate" version with vss in the name. Instead of igraph_vs_vector(), you could use igraph_vss_vector(), which returns a vertex selector instead of an error code. vss functions cannot fail, no need to check for errors. Using the vss version often simplifies code considerably.

szhorvat · 2024-05-08T15:29:14Z

plugins/graph_algorithm/src/algorithms/neighborhood.cpp

+            }
+
+            std::vector<std::vector<u32>> neighborhoods;
+            for (u32 i = 0; i < igraph_vector_int_list_size(&neighborhoods_raw); i++)


Another tip:

Generally, repeated calls to igraph's _size functions in a for loop can be detrimental to performance. This is because these C functions cannot be inlined, so the compiler generates a function call for each. The workaround is to call igraph_vector_int_list_size() once, save the result in a variable, and use that.

We do try to mitigate this situation, and annotate such functions with GCC's __attribute__((pure)), but this is an imperfect solution that only works with certain compilers.

Of course the performance impact won't be relevant in all scenarios, and is very likely irrelevant in this specific case.

…ions to ease creation of a sequential graph abstraction

joern274 and others added 17 commits April 25, 2024 12:31

cmake adapted

3e38cd8

Code fixes from szhorvat when migrating to igraph_0.10.x

683dca5

start to restructure graph_algorithm plugin (disfunctional)

422e7ef

changed (S)CC API

e314276

extend netlist graph functionality

f3e1d45

add support for neighborhood computation

1188570

add python bindings

d0f3800

cleanup

a13fa3e

added functions to get numbers of vertices and edges

8281365

removed templates and added getters accepting igraph vector

1b93fc7

added constructor accepting igraph graph object

97fbce2

refactored neighborhood algorithm

12d39dd

added subgraph computation algorithm

0324065

added function to get all (used) vertices

0d30bfd

added computation of shortest paths

7f5a198

cleanup

308bfcd

increase version

6b2218d

SJulianS requested review from SimonKlx and joern274 May 8, 2024 13:48

SJulianS requested a review from nils1603 as a code owner May 8, 2024 13:48

test pydoc

b7aaeb3

szhorvat reviewed May 8, 2024

View reviewed changes

some cleanup

8339179

SJulianS mentioned this pull request May 8, 2024

Graph algorithms not yet compatible with multi-driven nets #255

Closed

updated changelog

084a91a

SJulianS requested a review from swallat as a code owner May 8, 2024 18:21

SJulianS and others added 4 commits May 10, 2024 09:38

Merge branch 'master' into feature/latest_igraph

ff35022

added function in preparation for new NetlistTraversalDecorator funct…

48a8b6f

…ions to ease creation of a sequential graph abstraction

Dependency from graph_algorithm to igraph added

9b0458a

Merge branch 'master' into feature/latest_igraph

07c2bab

Updated URI to download latest igraph version

fe2b511

joern274 merged commit 3c74239 into master May 21, 2024
4 checks passed

joern274 deleted the feature/latest_igraph branch May 21, 2024 13:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/latest igraph #564

Feature/latest igraph #564

SJulianS commented May 8, 2024

szhorvat left a comment

szhorvat May 8, 2024

szhorvat May 8, 2024

joern274 May 21, 2024

szhorvat May 21, 2024

szhorvat May 21, 2024

szhorvat May 21, 2024

joern274 May 21, 2024

szhorvat May 21, 2024

joern274 May 21, 2024

szhorvat May 8, 2024

SJulianS May 8, 2024

szhorvat May 8, 2024

SJulianS May 8, 2024

szhorvat May 8, 2024

szhorvat May 8, 2024

szhorvat May 8, 2024

szhorvat May 8, 2024

Feature/latest igraph #564

Feature/latest igraph #564

Conversation

SJulianS commented May 8, 2024

szhorvat left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment