Add graph theory partitioners and conservation mode by omari91 · Pull Request #19 · IEE-TUGraz/NPAP

omari91 · 2026-02-19T21:59:23Z

Summary

Brief description of what this PR does.

Changes

Change 1
Change 2

Testing

Describe how you tested your changes.

All tests pass: pytest
Code is formatted: ruff format .
No linting errors: ruff check .
Documentation updated (if applicable)

Related Issues

Closes #

for more information, see https://pre-commit.ci

omari91 · 2026-02-19T22:08:38Z

Summary:
Adds graph-theory partitioners plus a transformer-conservation aggregation mode with matching docs/presets so the workflow, aggregation physics, and visualization pipeline stay aligned.

Changes:
- Introduced SpectralPartitioning and CommunityPartitioning, registered them with PartitioningManager, and documented them under “Available Strategies”.
- Added TransformerConservationStrategy, exposed via AggregationMode.CONSERVATION, and documented the mode plus a new “Workflows” guide that shows how to combine community-aware partitioning with conservation-mode aggregation and preset-driven visualization.
- Extended npap.visualization with PlotPreset and a preset argument for plot_network, then documented the presets.
Testing: python -m pytest (243 tests); python -m ruff check . (0 lint errors).

MarcoAnarmo

Hey @omari91! First of all, thank you so much for taking the time to put this together — graph-theory partitioners and the transformer conservation mode are awesome additions and really align well with NPAP 😃

I've gone through all your changes and left some inline comments. Here's a quick summary of the broader things:

Documentation: The new classes (SpectralPartitioning, CommunityPartitioning, TransformerConservationStrategy, PlotPreset) would need entries in the API reference files under docs/api/ so they get picked up by the auto-generated documentation. That way users can discover them easily alongside the existing strategies. Also make sure the docstrings follow the NumPy style we use throughout. You can check geographical.py or basic_strategies.py for reference!
Tests: You've got good coverage on the happy paths already, which is great. It would be nice to add a couple more cases to make things really solid, like an end-to-end test for AggregationMode.CONSERVATION, and what happens when there are no transformers in the graph.
Commit practices: Just a small tip for future PRs: it really helps if you work on a feature branch (something like feature/graph-theory-partitioners) instead of pushing from your main. It makes it way easier to go back and forth during reviews. Splitting the work into smaller commits (one for partitioning, one for conservation, one for visualization, etc) also helps a lot when reviewing. No big deal for this one though!

Really appreciate the effort here. Let me know if you have any questions about the comments. Happy to chat through any of them, and thank you so much for your time!😄

MarcoAnarmo · 2026-02-20T11:54:27Z

npap/aggregation/physical_strategies.py

+    def _calculate_equivalent(self, edges: list[dict[str, Any]], prop: str) -> float | None:
+        try:
+            return self._equivalent_reactance.aggregate_property(edges, prop)
+        except AggregationError:
+            return None


This silently returns None when the equivalent reactance calculation fails. The AggregationError is caught and swallowed with no logging. This could hide real problems (e.g., missing properties). Please add a log_warning when returning None due to a caught exception, so users can diagnose issues.

Adjusted _calculate_equivalent() so it logs a warning whenever AggregationError is caught before returning None. The warning includes the property name and exception so users see when impedance aggregation failed (e.g., missing attributes), rather than silently dropping the value.

MarcoAnarmo · 2026-02-20T11:59:42Z

npap/aggregation/physical_strategies.py

+        node_to_cluster = build_node_to_cluster_map(partition_map)
+        cluster_edge_map = build_cluster_edge_map(original_graph, node_to_cluster)


These two mappings are already computed by AggregationManager.aggregate() in managers.py:272-273, but aren't passed down to the physical strategy. This means every call to TransformerConservationStrategy recomputes them from scratch.
The pre-computed maps could be forwarded via the parameters dict.

Now passing the pre-computed node_to_cluster and cluster_edge_map into the physical strategy via profile.physical_parameters before calling aggregate(). TransformerConservationStrategy uses those maps if provided, falling back to recomputing only if they’re missing, so we avoid redundant work.

MarcoAnarmo · 2026-02-20T12:03:21Z

npap/interfaces.py

Looks good. Minor note: the existing required_topology validation in managers.py:292 compares topology_strategy.__class__.__name__ against physical_strategy.required_topology, which means it compares "ElectricalTopologyStrategy" to "electrical". They will never match, producing a false warning every time CONSERVATION mode is used.

This is a pre-existing bug, but your PR is the first to trigger it. Could you fix the check in managers.py:292 to use profile.topology_strategy instead of topology_strategy.__class__.__name__?

Fixed: AggregationManager.aggregate() now compares profile.topology_strategy (string like "electrical") against physical_strategy.required_topology, so the warning only fires when the configured topology string actually differs from what the physical strategy expects. This prevents the false warning that appeared when running CONSERVATION mode.

MarcoAnarmo · 2026-02-20T12:05:29Z

npap/partitioning/graph_theory.py

+class CommunityPartitioning(PartitioningStrategy):
+    """
+    Partition using greedy modularity community detection.
+
+    This strategy uses NetworkX's ``greedy_modularity_communities`` routine to detect communities
+    and is deterministic for a given graph structure.
+    """
+
+    def required_attributes(self) -> dict[str, list[str]]:
+        return {"nodes": [], "edges": []}
+
+    def _strategy_name(self) -> str:
+        return "community_modularity"
+
+    def partition(self, graph: nx.DiGraph, **kwargs) -> dict[int, list[Any]]:
+        communities = list(greedy_modularity_communities(graph.to_undirected()))
+
+        if not communities:
+            raise PartitioningError(
+                "Community detection returned no communities.",
+                strategy=self._strategy_name(),
+            )
+
+        partition_map = {idx: list(cluster) for idx, cluster in enumerate(communities)}
+        validate_partition(partition_map, graph.number_of_nodes(), self._strategy_name())
+
+        return partition_map


Since greedy modularity determines the number of clusters automatically, what happens if a user passes n_clusters? It will be silently ignored. Please either emit a warning when n_clusters is provided (e.g., log_warning("community_modularity determines cluster count automatically, n_clusters is ignored")) or document this clearly in the docstring.

CommunityPartitioning.partition() now checks for n_clusters in its kwargs and emits a log_warning stating that the modularity-based routine determines cluster count automatically, so n_clusters is ignored. The docstring also now calls this out. Tests cover the warning via caplog.

MarcoAnarmo · 2026-02-20T12:06:50Z

npap/partitioning/graph_theory.py

+class SpectralPartitioning(PartitioningStrategy):
+    """
+    Partition using spectral clustering on the graph Laplacian.
+
+    This strategy converts the input graph into a symmetric adjacency matrix and applies
+    ``sklearn.cluster.SpectralClustering`` with ``affinity="precomputed"``. Use this when
+    you need community-aware partitions on graphs that are not easily handled by geometric
+    distances (e.g., line topology with weak geographic structure).
+
+    Parameters
+    ----------
+    random_state : int | None
+        Seed for reproducible k-means assignment inside spectral clustering.
+    """


The existing partitioning strategies all document their AC-island awareness behavior. Could you add a note about how spectral clustering interacts with AC islands? Since it operates on the adjacency matrix, it should naturally respect graph connectivity, but it would be good to document this explicitly for consistency with the other strategies :)

Added the requested note to SpectralPartitioning’s docstring: it now explicitly says the adjacency matrix is derived from the graph connectivity, so disconnected AC islands stay separate clusters and the strategy respects island boundaries automatically. This keeps it consistent with the other partitioners’ documentation.

MarcoAnarmo · 2026-02-20T12:10:37Z

npap/visualization.py

The current plot_network() function creates a PlotConfig(**kwargs), but PlotConfig has no preset field. If a user passes preset=PlotPreset.PRESENTATION as shown in the workflows doc, this would raise a TypeError.

Please verify this works end-to-end. The preset likely needs to be popped from kwargs and applied before creating the PlotConfig.

the implementation now pops any preset (and accepts PlotPreset or string names) before building the PlotConfig, applies _apply_preset_overrides, and only then merges in the remaining kwargs. So calling plot_network(..., preset=PlotPreset.PRESENTATION) no longer fails, and the workflows example works end-to-end. The change has been tested via python -m pytest and python -m ruff check .

codecov · 2026-02-20T17:54:52Z

Codecov Report

❌ Patch coverage is 67.15328% with 45 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
npap/visualization.py	0.00%	31 Missing ⚠️
npap/aggregation/physical_strategies.py	85.10%	5 Missing and 2 partials ⚠️
npap/partitioning/graph_theory.py	86.36%	4 Missing and 2 partials ⚠️
npap/managers.py	88.88%	0 Missing and 1 partial ⚠️

📢 Thoughts on this report? Let us know!

for more information, see https://pre-commit.ci

…rk data

omari91

@MarcoAnarmo have a look at this update for review

omari91 and others added 2 commits February 19, 2026 22:55

Add graph theory partitioners and conservation mode

606fbee

[pre-commit.ci] auto fixes from pre-commit.com hooks

4a2f49a

for more information, see https://pre-commit.ci

IEE-TUGraz deleted a comment from codecov bot Feb 20, 2026

MarcoAnarmo reviewed Feb 20, 2026

View reviewed changes

Address review comments

3b04ea7

omari91 and others added 20 commits February 21, 2026 00:19

Merge branch 'IEE-TUGraz:main' into main

d2a8178

Remove empty roadmap link from feature template

7a459cd

Add graph copy helper

c83ebc4

feature enhancement: copy graph visualization

d33cf5a

Add property-based partitioning tests

83b75b0

Add CLI helpers

3a1df80

Add PTDF/Kron diagnostics

88684d8

feature update

bf4eaca

[pre-commit.ci] auto fixes from pre-commit.com hooks

15c98cc

for more information, see https://pre-commit.ci

Add tests to cover CLI and aggregation helpers

5c46e48

[pre-commit.ci] auto fixes from pre-commit.com hooks

61292c7

for more information, see https://pre-commit.ci

Fix CSV loading: remove hardcoded quotechar and add comment support

f024ddb

[pre-commit.ci] auto fixes from pre-commit.com hooks

846ae13

for more information, see https://pre-commit.ci

feature enhancement: copy graph visualization

942027d

Add CLI helpers

bff3201

[pre-commit.ci] auto fixes from pre-commit.com hooks

245cf7a

for more information, see https://pre-commit.ci

Add tests to cover CLI and aggregation helpers

f72a7b2

[pre-commit.ci] auto fixes from pre-commit.com hooks

a0f93f0

for more information, see https://pre-commit.ci

Include debug scripts for Zenodo data loading

e28016c

docs: cleanup .hypothesis and add example library with European netwo…

943c1f6

…rk data

omari91 force-pushed the main branch from bc6613c to 6b5ecf4 Compare February 26, 2026 04:16

chore: formatting and pypsa related test failure

1088e29

omari91 force-pushed the main branch from 6b5ecf4 to 1088e29 Compare February 26, 2026 04:22

omari91 commented Feb 26, 2026

View reviewed changes

		node_to_cluster = build_node_to_cluster_map(partition_map)
		cluster_edge_map = build_cluster_edge_map(original_graph, node_to_cluster)

Conversation

omari91 commented Feb 19, 2026

Summary

Changes

Testing

Related Issues

Uh oh!

omari91 commented Feb 19, 2026

Uh oh!

MarcoAnarmo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Feb 20, 2026

Codecov Report

Uh oh!

omari91 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants