Merge links on nodes: create super nodes by mgovers · Pull Request #1398 · PowerGridModel/power-grid-model

mgovers · 2026-05-13T14:24:31Z

Part of #35

This actually creates the topological nodes from the regular nodes

Signed-off-by: Martijn Govers <Martijn.Govers@Alliander.com>

mgovers · 2026-05-13T14:27:20Z

+struct ReducedComponentTopology {
+    Idx n_node{}; // num of topological nodes (with reduced links), plus internal nodes for 3-way branches
+    std::vector<BranchIdx> branch_node_idx;
+    std::vector<Branch3Idx> branch3_node_idx;
+    IdxVector shunt_node_idx;
+    IdxVector source_node_idx;
+    IdxVector load_gen_node_idx;
+    std::span<LoadGenType const> load_gen_type;
+    IdxVector voltage_sensor_node_idx;
+    std::span<Idx const> power_sensor_object_idx; // the index is relative to branch, source, shunt or load_gen
+    std::span<MeasuredTerminalType const> power_sensor_terminal_type;
+    std::span<Idx const> current_sensor_object_idx; // the index is relative to branch
+    std::span<MeasuredTerminalType const> current_sensor_terminal_type;
+    std::span<ComponentType const> regulator_type;
+    std::span<Idx const> regulated_object_idx; // the index is relative to branch or branch3
+    std::span<ComponentType const> regulated_object_type;


the span ones essentially do not change when we go from user nodes to topological nodes, so we can do with a pure view. The branch2, branch3 and appliance ones may change so they need a pass-by-copy.

We can also opt for a maybe_owning_view, which can either take by reference, or by value, depending on the situation. I'm investigating that on the side

mgovers · 2026-05-18T06:24:22Z

+#if __cpp_lib_ranges_pairwise >= 202110L
+using std::views::pairwise;
+#else
+constexpr auto pairwise(std::ranges::viewable_range auto&& range_to_pair) {
+    auto const begin = std::ranges::begin(range_to_pair);
+    auto const end = std::ranges::end(range_to_pair);
+    if (begin == end) {
+        return std::views::zip(std::ranges::subrange(begin, begin), std::ranges::subrange(begin, begin));
+    }
+    return std::views::zip(std::ranges::subrange(begin, end - 1), std::ranges::subrange(begin + 1, end));
+}
+#endif


in the end, I did not use it in this PR but initially i thought i needed it. I've had the same thing in the past already (multiple times) but removed it every single time, so I was like "ok, might as well keep it this time"

I think its a handy function. I see we can already use it in sparse_decode in grouped_idx_vector maybe. Uses similar logic.

It's constexpr and we shall fully have it when c++23 is fully supported + it's handy. Let's keep it.

nitbharambe · 2026-05-19T08:02:49Z

+    explicit TopologicalNodeMapping(Idx n_topo_nodes_, IdxVector mapping)
+        : mapping_{std::move(mapping)}, n_topo_nodes_{n_topo_nodes_} {


Suggested change

explicit TopologicalNodeMapping(Idx n_topo_nodes_, IdxVector mapping)

: mapping_{std::move(mapping)}, n_topo_nodes_{n_topo_nodes_} {

explicit TopologicalNodeMapping(Idx n_topo_nodes, IdxVector mapping)

: mapping_{std::move(mapping)}, n_topo_nodes_{n_topo_nodes} {

figueroa1395 · 2026-05-19T13:27:02Z

+#if __cpp_lib_ranges_pairwise >= 202110L
+using std::views::pairwise;
+#else
+constexpr auto pairwise(std::ranges::viewable_range auto&& range_to_pair) {
+    auto const begin = std::ranges::begin(range_to_pair);
+    auto const end = std::ranges::end(range_to_pair);
+    if (begin == end) {
+        return std::views::zip(std::ranges::subrange(begin, begin), std::ranges::subrange(begin, begin));
+    }
+    return std::views::zip(std::ranges::subrange(begin, end - 1), std::ranges::subrange(begin + 1, end));
+}
+#endif


It's constexpr and we shall fully have it when c++23 is fully supported + it's handy. Let's keep it.

figueroa1395 · 2026-05-19T13:33:34Z

+};
+
+struct ReducedComponentTopology {
+    Idx n_node{}; // num of topological nodes (with reduced links), plus internal nodes for 3-way branches


The "(with reduced links)" part makes this confusing. It should be all topological nodes, which includes user nodes not connected by links and all supernodes, right?

figueroa1395 · 2026-05-19T13:36:29Z

 struct ComponentConnections {
    std::vector<BranchConnected> branch_connected;
    std::vector<Branch3Connected> branch3_connected;
+    std::vector<BranchConnected> link_connected;


For this struct and for ComponentTopology, let's add a short comment on why we have split links from branches - for future reference.

figueroa1395 · 2026-05-19T13:41:04Z

+    }
+
+    auto mapping() const& -> IdxVector const& { return mapping_; }
+    auto mapping() && -> IdxVector { return std::move(mapping_); }


Maybe I'll find out later, but why not return a span? Why explicitly define the move away option?

figueroa1395 · 2026-05-19T13:45:15Z

+namespace power_grid_model::supernodes {
+class TopologicalNodeMapping {
+  public:
+    TopologicalNodeMapping() = default;


Why the default constructor when below you have an explicit one and there is no way to later "populate" the class? I'd argue having a default one makes it easier for us to introduce a bug by mistake. Thoughts?

figueroa1395 · 2026-05-20T05:27:54Z

+                              auto const from = vertices[0];
+                              auto const to = vertices[1];


Maybe a sanity check like

assert(from < n_nodes && to < n_nodes);

for when we split links from branches in main_core/topology.hpp?

figueroa1395 · 2026-05-20T05:33:29Z

+// not the user nodes. This effectively removes all link-related stuff from the math topology.
+inline ReducedComponentTopology construct_reduced_topology(ComponentTopology const& comp_topo,
+                                                           TopologicalNodeMapping const& topo_node_mapping) {
+    auto remap_nodes = [&topo_node_mapping]<typename IdxType>(std::vector<IdxType> const& user_nodes) {


Clever stuff, nicely done!

figueroa1395 · 2026-05-20T05:53:38Z

+            if (link_connected[0] == 0 || link_connected[1] == 0) {
+                return Idx2D{.group = disconnected, .pos = disconnected};
+            }


I'm thinking about this edge case:
If we have a link that's disconnected on one side, but it's not part of any other super node, and its connected side is then connected to a node to an active line, what will happen to that link? It clearly isn't part of a supernode so it will be excluded from the link solver algorithm, but it will also be excluded from the regular topology. What happens then? Is this expected? We still have to have output for it on the connected side.

figueroa1395 · 2026-05-20T05:55:39Z

+            REQUIRE(topo_node_mapping.n_user_nodes() == comp_topo.n_node);
+            CHECK(topo_node_mapping.mapping() == IdxVector{0, 0, 1});
+        }
+        SUBCASE("Disconnected link => not remapped") {


The mentioned edge case in my previous comment is similar to this, however, can you add it here as well too? Just to make sure the mapping handles it well.

figueroa1395 · 2026-05-20T05:56:12Z

+    SUBCASE("construct_reduced_topology") {
+        // TODO: Add test implementation
+    }
+    SUBCASE("reduce_topology") {
+        // TODO: Add test implementation
+    }


Reminder to add these :)

nitbharambe · 2026-05-20T09:03:02Z

+                                                  std::views::transform([&topo_nodes](auto const& idx_and_topo) {
+                                                      auto const& [user_node, topo_node] = idx_and_topo;
+                                                      topo_nodes[topo_node].user_nodes.push_back(user_node);
+                                                      return Idx2D{.group = topo_node, .pos = user_node};


Maybe we can already start with separate alias for same type.

I already might be getting confused with the terminology of user / topo / supernodes (And then maybe we need to rename or define the terms in comments.)

nitbharambe · 2026-05-20T09:09:27Z

    Idx n_node{};
    std::vector<BranchIdx> branch_node_idx;
    std::vector<Branch3Idx> branch3_node_idx;
+    std::vector<BranchIdx> link_node_idx;


This comment is intended for future refactor:

cfr. https://github.com/PowerGridModel/power-grid-model/pull/1398/changes#r3272590347

Lets define what index means.
PGM maintainers can understand that these links are ordered on the basis of their order in container. But maybe not new contributors.

I say this because now there might be overlap in content between branch and link.

nitbharambe · 2026-05-22T08:22:36Z

no new comments from me @mgovers

mgovers added 8 commits April 29, 2026 14:23

topology to separate namespace

935b71f

Signed-off-by: Martijn Govers <Martijn.Govers@Alliander.com>

skeleton for supernodes

cf807ef

Signed-off-by: Martijn Govers <Martijn.Govers@Alliander.com>

create map

7e0d286

Signed-off-by: Martijn Govers <Martijn.Govers@Alliander.com>

refactor

d668c39

Signed-off-by: Martijn Govers <Martijn.Govers@Alliander.com>

refactor

716e1dd

Signed-off-by: Martijn Govers <Martijn.Govers@Alliander.com>

rewrite to not use grouped idx vector

60bc575

Signed-off-by: Martijn Govers <Martijn.Govers@Alliander.com>

create supernodes

4ed7f7a

Signed-off-by: Martijn Govers <Martijn.Govers@Alliander.com>

complete implementation of construct_reduced_topology

ec4b96d

Signed-off-by: Martijn Govers <Martijn.Govers@Alliander.com>

mgovers requested review from figueroa1395 and nitbharambe May 13, 2026 14:24

mgovers self-assigned this May 13, 2026

mgovers added the feature New feature or request label May 13, 2026

mgovers commented May 13, 2026

View reviewed changes

mgovers commented May 18, 2026

View reviewed changes

nitbharambe reviewed May 19, 2026

View reviewed changes

figueroa1395 reviewed May 20, 2026

View reviewed changes

nitbharambe reviewed May 20, 2026

View reviewed changes

		explicit TopologicalNodeMapping(Idx n_topo_nodes_, IdxVector mapping)
		: mapping_{std::move(mapping)}, n_topo_nodes_{n_topo_nodes_} {

Conversation

mgovers commented May 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

figueroa1395 May 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

nitbharambe commented May 22, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

mgovers commented May 13, 2026 •

edited

Loading

figueroa1395 May 20, 2026 •

edited

Loading