Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature/girgs #1142

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Feature/girgs #1142

wants to merge 2 commits into from

Conversation

chistopher
Copy link

I tried to add a generator for GIRGs.
In contrast to #320, this PR is just a wrapper around our implementation and I tried to keep the code complexity as low as possible.
The GIRGs repo is added as a git submodule and builds a static library.
Networkit or its components link against it but do not expose it further.

There still remain some issues that I don't like and I would greatly appreciate your input.

  • in a non-monolith build, all components link against GIRGs although only the generators module needs it
  • GIRGs enables -Wswitch-default warnings that trigger a lot here. I can remove this from the public interface of GIRGs if desired.
  • the algorithm does not catch SIGINT. Since the implementation is very fast, we could catch SIGINT in the wrapper and just ignore it to prevent an interactive python shell from exiting.

@fabratu
Copy link
Member

fabratu commented Nov 6, 2023

If I have not missed something, -Wswitch-default only triggers for the edge iterators. I would rather not add a default case, since these are exhaustive switch cases. If that's not a problem, I would recommend to disable it as already suggested.

Given the time, I will read into #320 in the next days and then give further implications on how we proceed with the integration.

@fabratu fabratu self-assigned this Nov 6, 2023
@fabratu
Copy link
Member

fabratu commented Feb 27, 2024

While we can fix the first and third of your points, I am still a bit indecisive on what to do with this feature integration in general. After looking at #320, the decision was to add the generator as an external library and I can see pros and cons about it.

Can you maybe elaborate more on the following topics?

  • Are there any plans to continue development on the GIRGS/HyperGIRGS tool?
  • Given the integration into NetworKit was started, how much effort would it be to still do it?
  • Is there a possibility to pass a function as a parameter to girgs::generateEdges, so that edges are getting passed to that function/handle? In that way, we might be able to remove the doubling of edges in memory.

Example without sanity checks (given uand v represent the generated edge ids):

template <typename L, typename T>
generateEdges(..., L handle) {
...
L(static_cast<T>(u), static_cast<T>(v));
...
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants