Skip to content

Merge links into nodes: Create link output from link solver algorithm - #1545

Open
figueroa1395 wants to merge 32 commits into
mainfrom
pgm/feature/link-supernode-output-enabled
Open

Merge links into nodes: Create link output from link solver algorithm#1545
figueroa1395 wants to merge 32 commits into
mainfrom
pgm/feature/link-supernode-output-enabled

Conversation

@figueroa1395

Copy link
Copy Markdown
Member

Part of #35

Follow up of #1536

This PR should activate the Link output with two paths (old and new) + add the steady state output for i

Signed-off-by: Santiago Figueroa Manrique <santiago.figueroa.manrique@alliander.com>
@figueroa1395
figueroa1395 requested a review from mgovers August 21, 2026 10:31
@figueroa1395 figueroa1395 self-assigned this Aug 21, 2026
@figueroa1395 figueroa1395 added feature New feature or request do-not-merge This should not be merged labels Aug 21, 2026
mgovers and others added 10 commits August 24, 2026 13:56
Signed-off-by: Martijn Govers <Martijn.Govers@Alliander.com>
Signed-off-by: Martijn Govers <Martijn.Govers@Alliander.com>
Signed-off-by: Martijn Govers <Martijn.Govers@Alliander.com>
Signed-off-by: Martijn Govers <Martijn.Govers@Alliander.com>
@mgovers
mgovers marked this pull request as ready for review August 25, 2026 09:11
…k-supernode-output-enabled

Signed-off-by: Martijn Govers <martijn.govers@alliander.com>
mgovers and others added 2 commits August 26, 2026 09:51
…k-supernode-output-enabled

Signed-off-by: Martijn Govers <martijn.govers@alliander.com>
Signed-off-by: Martijn Govers <Martijn.Govers@Alliander.com>
@mgovers mgovers removed the do-not-merge This should not be merged label Aug 26, 2026
Base automatically changed from pgm/feature/link-supernode-output to main August 26, 2026 14:28
Signed-off-by: Martijn Govers <Martijn.Govers@Alliander.com>
Comment thread tests/cpp_unit_tests/main_core/test_topological_node_output.cpp Outdated
nitbharambe
nitbharambe previously approved these changes Aug 27, 2026

@nitbharambe nitbharambe left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Rest looks good

Signed-off-by: Martijn Govers <Martijn.Govers@Alliander.com>
Signed-off-by: Martijn Govers <Martijn.Govers@Alliander.com>
Signed-off-by: Martijn Govers <Martijn.Govers@Alliander.com>
@mgovers
mgovers requested a balanced review from Copilot and removed request for mgovers August 27, 2026 14:55
Signed-off-by: Martijn Govers <martijn.govers@alliander.com>

Copilot AI left a comment

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.

Pull request overview

Adds link outputs for merged-node topology while retaining legacy output behavior.

Changes:

  • Generates steady-state and short-circuit link outputs.
  • Includes shunts and three-winding transformers in link-flow balancing.
  • Adds component traits and expanded unit tests.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
topological_node_output.hpp Accumulates grid flows and generates link outputs.
output.hpp Routes legacy and merged-topology link outputs.
container.hpp Exposes stored component types.
test_topological_node_output.cpp Tests flow accumulation and link solving.
test_main_core_output.cpp Tests link output conversion.
Suppressed comments (2)

power_grid_model_c/power_grid_model/include/power_grid_model/main_core/output.hpp:147

  • The short-circuit path repeats the incorrect energized test: it marks a supplied one-sided link off and a closed link in an unsupplied island on. Guard a disconnected topo_id, then use state.topo_comp_coup->node[topo_id.group] to determine source connectivity, matching the legacy branch path and Edge::energized semantics.
    if (!link.edge_status() || topo_id.group == disconnected) {

tests/cpp_unit_tests/main_core/test_main_core_output.cpp:285

  • As in the steady-state case, a link with one active terminal connected to a source should remain energized; only its flow is zero. This status_off expectation conflicts with Edge::energized and with the old output path. Add supplied node-to-math coupling to the fixture and expect IntS{1}.
            CHECK(output[1].id == 1);
            CHECK(output[1].energized == status_off);

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread power_grid_model_c/power_grid_model/include/power_grid_model/main_core/output.hpp Outdated
Comment thread tests/cpp_unit_tests/main_core/test_main_core_output.cpp Outdated
Signed-off-by: Martijn Govers <Martijn.Govers@Alliander.com>
Signed-off-by: Martijn Govers <Martijn.Govers@Alliander.com>
Signed-off-by: Martijn Govers <Martijn.Govers@Alliander.com>
Signed-off-by: Martijn Govers <Martijn.Govers@Alliander.com>
Signed-off-by: Martijn Govers <Martijn.Govers@Alliander.com>
Signed-off-by: Martijn Govers <Martijn.Govers@Alliander.com>
Signed-off-by: Martijn Govers <Martijn.Govers@Alliander.com>
Comment thread .pre-commit-config.yaml
Signed-off-by: Martijn Govers <Martijn.Govers@Alliander.com>

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

i realized that there's no test case checking link energized. as a result, we almost introduced a bug in this PR in which a link connected only on one side to source would be marked "energized": 0. That is of course incorrect. Its p_(from|to) and q_(from|to) should be 0 but not the energized. Hence I added this validation case

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

Copy link
Copy Markdown

@nitbharambe nitbharambe added the do-not-merge This should not be merged label Aug 28, 2026
@nitbharambe

Copy link
Copy Markdown
Member

do-not-merge added since its friday. Rest looks good

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

Labels

do-not-merge This should not be merged feature New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants