Skip to content

Implement InducedSubgraphView#43

Open
IanChenUIUC wants to merge 11 commits into
Ladybug-Memory:mainfrom
IanChenUIUC:feature/induced-subgraph-view
Open

Implement InducedSubgraphView#43
IanChenUIUC wants to merge 11 commits into
Ladybug-Memory:mainfrom
IanChenUIUC:feature/induced-subgraph-view

Conversation

@IanChenUIUC

Copy link
Copy Markdown
Contributor

Implementation for issue #39

@IanChenUIUC IanChenUIUC marked this pull request as draft June 25, 2026 22:14
Comment thread include/networkit/graph/InducedSubgraphView.hpp Outdated
@IanChenUIUC

Copy link
Copy Markdown
Contributor Author

Remaining items to implement are:

  • option for compact IDs (so that it provides the same options as the GraphTools::SubgraphFromNodes)
  • a few trailing test cases, namely the edge iterator and checking constructors
  • adding cython bindings for InducedSubgraphView

Something to note is that I had to make the hasNode and hasEdge functions virtual, so that my subclass could override them. This may have some performance costs for the rest of the codebase, but I have not benchmarked anything.

I think we should save the base GraphView class, which InducedGraphView and CoarsenedGraphView inherit from, for a different PR, as this one is already becoming large.

@IanChenUIUC IanChenUIUC marked this pull request as ready for review June 29, 2026 21:32
@IanChenUIUC

Copy link
Copy Markdown
Contributor Author

I'm looking at the GraphView interface that Arun proposed above, and I think I like the idea to not compute the degrees eagerly, and instead have the user explicitly call an invalidateCache (e.g. a synchronization function). Any other ideas?

@adsharma

Copy link
Copy Markdown
Contributor

Or do it on the first request to access the degrees

@IanChenUIUC

Copy link
Copy Markdown
Contributor Author

Or do it on the first request to access the degrees

Sounds good. I'll start working on adding that behavior.

// For mutable graphs, check exists
#pragma omp parallel for reduction(+ : sum)
for (omp_index v = 0; v < static_cast<omp_index>(z); ++v) {
if (exists[v]) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs an updated for InducedSubgraphView?

originalGraph.get().forInNeighborsOf(v, [&](node u, edgeweight ew) {
if (!hasNode(u) || u == v)
return;
++inducedDegree[u];

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This keeps m from double-counting, but makes the public in-degree wrong for directed self-loops. Fixing this likely needs separate edge-count delta logic on removal too, since networkit/cpp/graph/InducedSubgraphView.cpp:119 currently subtract out-degree plus cached in-degree.

build:
name: "${{ matrix.os }} (${{ matrix.compiler }})"
runs-on: ${{ matrix.runner }}
if: github.event_name != 'pull_request' || !github.event.pull_request.draft

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is this for? If necessary break it into a separate PR

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did not want every commit on a draft PR to introduce a new CI workflow. Hmm, I can break it into a separate PR.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sometimes people want to use draft commits mainly for the CI. You can probably use this method to [skip ci]:

https://docs.github.com/en/actions/how-tos/manage-workflow-runs/skip-workflow-runs

* Copy assignment
*/
InducedSubgraphView &operator=(const InducedSubgraphView &other) {
Graph::operator=(other);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs to handle originalGraph, nodeSubset, inducedDegree, and inducedInDegree etc

@adsharma

adsharma commented Jul 2, 2026

Copy link
Copy Markdown
Contributor

performance: Full edge traversal of a small view scans the entire base graph. networkit/cpp/graph/InducedSubgraphView.cpp:198 and networkit/cpp/graph/InducedSubgraphView.cpp:225 call originalGraph.forEdges/parallelSumForEdges and filter with hasEdge, making view traversal O(E_base) plus set lookups. For induced views, this should iterate nodeSubset and filter each selected node’s neighbors, with undirected deduplication.

Also as an integration test, please run the abm14 graph and see if any runtime regressed due to the virtual keyword for some of the methods made them expensive. Another area to watch out is that some of the algorithms use virtual methods and at other times templated methods to access the graph.

@adsharma

adsharma commented Jul 2, 2026

Copy link
Copy Markdown
Contributor

Details on the virtual vs templated. Only one problematic instance was found in the review.

The nuance is that C++ templates are only “polymorphic” through the operations they call inside the instantiated base template. For example, Graph::forNodes is a template, so if code has:

const Graph &g = view;
g.forNodes(...)

it instantiates/runs Graph::forNodes, not InducedSubgraphView::forNodes. The branch partially mitigates that by changing many internal checks from exists[v] to virtual hasNode(v), so those base-template paths become correct but potentially slow.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants