Unit test not observable network passed through no perturbation by Jerry-Jinfeng-Guo · Pull Request #1322 · PowerGridModel/power-grid-model

Jerry-Jinfeng-Guo · 2026-03-06T19:12:29Z

Added unit tests for checking the possibility to use perturbation due to observability check render .is_observable state without exception thrown.

PR comes from #1317

Signed-off-by: Jerry Jinfeng Guo <jerry.jinfeng.guo@alliander.com>

Copilot

Pull request overview

Adds C++ unit tests around math_solver::observability::ObservabilityResult::use_perturbation() to ensure non-observable networks don’t trigger perturbation, and to exercise the use_perturbation boolean logic.

Changes:

Added a new TEST_CASE covering use_perturbation() for a non-observable meshed network with multiple voltage phasor sensors.
Added additional subcases attempting to cover an observable case and direct ObservabilityResult logic combinations.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

tests/cpp_unit_tests/test_observability.cpp

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Signed-off-by: Jerry Guo <6221579+Jerry-Jinfeng-Guo@users.noreply.github.com>

Signed-off-by: Jerry Jinfeng Guo <jerry.jinfeng.guo@alliander.com>

figueroa1395 · 2026-03-09T09:35:40Z

tests/data/state_estimation/meshed-network-observability/README.md

+Test case `07-observable-2-voltage-sensors` represents a meshed network with two voltage phasor sensors
+(voltage measurements with both magnitude and angle). This configuration is currently treated as
+**non-observable** due to a known limitation: the current meshed network sufficient-condition
+implementation cannot handle multiple voltage phasor sensors.


I think that this description is misleading. Because even though we deem it unobservable internally, we don't throw NotObservableError by design. Hence, if it's truly unobservable, in this case we would reach a SparseMatrixError, but since this specific case is actually observable even if it has two voltage phasor sensors, the calculation proceeds as expected.

To me this description doesn't reflect what I wrote above.

figueroa1395 · 2026-03-09T09:36:32Z

tests/data/state_estimation/meshed-network-observability/README.md

+This edge case is comprehensively covered by unit tests in `tests/cpp_unit_tests/test_observability.cpp`
+(specifically the test case "Test ObservabilityResult - use_perturbation with non-observable network").
+The unit tests verify:
+
+- Networks with `n_voltage_phasor_sensors > 1 && !is_radial` are correctly identified as non-observable
+- The `ObservabilityResult.is_observable` flag is set to `false`
+- The `use_perturbation()` method returns `false` (no perturbation applied to non-observable networks)
+- The specific code path in `observability_check()` that triggers early return for this condition


This is true and already well described in the unit test. I don't think we need to explicitly write such a description here.

figueroa1395 · 2026-03-09T09:38:33Z

tests/data/state_estimation/meshed-network-observability/README.md

+When support for multiple voltage phasors in meshed networks is implemented, the unit tests can be
+updated accordingly, and test case `07-observable-2-voltage-sensors` can be populated with expected
+output values.


Actually, this test can already be populated with expected output values, even with current implementation (I described the reason in https://github.com/PowerGridModel/power-grid-model/pull/1322/changes#r2904270623). The reason we don't have output values here is because we just use it to test that we can process such a case that is actually observable even thought voltage phasor sensors are present (@nitbharambe correct me if I'm wrong please).

figueroa1395 · 2026-03-09T09:43:05Z

tests/cpp_unit_tests/test_observability.cpp

+        topo.power_sensors_per_load_gen = {from_sparse, {0}};
+        topo.power_sensors_per_shunt = {from_sparse, {0}};
+        // Sufficient branch sensors to pass necessary condition
+        topo.power_sensors_per_branch_from = {from_sparse, {0, 1, 2, 3, 4}};


Why are these sensors needed if there aren't any loads? Having 0 power sensors should satisfy the necessary condition in this scenario.

figueroa1395 · 2026-03-09T09:45:11Z

tests/cpp_unit_tests/test_observability.cpp

+        CHECK(result.is_observable == false);
+
+        // Verify that use_perturbation() returns false when not observable
+        CHECK(result.use_perturbation() == false);


I think this behavioral check is good and valuable.

figueroa1395 · 2026-03-09T09:53:05Z

tests/data/state_estimation/meshed-network-observability/README.md

+An explicit integration test marked as "expected to fail" is **not necessary** because:
+
+1. **Unit test coverage is sufficient**: The unit tests provide precise, maintainable verification at the
+   appropriate level of granularity.
+2. **Maintenance burden**: "Expected to fail" tests require special infrastructure and documentation,
+   and risk being forgotten when the limitation is eventually addressed.
+3. **Rare use case**: Multiple voltage phasor sensors (with precise angle measurements) in meshed
+   networks are uncommon in real-world applications.


I don't think we need such a description here, but I also disagree with some things mentioned:
2. A test case that is expected to fail doesn't need special handling beyond the additional parameter. Moreover, they are good because they are hard to miss. If there is a behavioral change and the expected to fail test now passes, then such test would now fail, making it clear that we either have to change it according to the new behavior or that we messed up somewhere.
3. I don't think this is relevant here.

What I had envisioned when I suggested the new test in #1317 (comment), was different compared to 07-observable-2-voltage-sensors. The mentioned one, doesn't need a perturbation to perform a successful calculation, whereas what I intended to propose, would be a similar meshed test (probably with links involved), that even though marked as non-observable internally (because of the >2 voltage phasor voltages present), it would run the calculation but fail with a SparseMatrixError because perturbation would be needed (but we don't allow it because of .is_observable = false, not because it is not observable "in real life"). Nevertheless, this might be a hassle to come up with and merging links into nodes would probably deem it unnecessary, so I think it's not needed unless others think otherwise.

figueroa1395 · 2026-03-09T09:54:23Z

tests/data/state_estimation/meshed-network-observability/README.md

-1. Observable system with branch measurement.
-2. Observable system without branch measurement (using only nodal measurements).
-3. Unobservable system with branch measurement (x2).
+The following test cases are included:


Nice addition, easy to find out what's what. Maybe good to add such descriptions in other places as well. Out of scope, just food for thought.

figueroa1395 · 2026-03-09T09:56:05Z

tests/cpp_unit_tests/test_observability.cpp

+        CHECK(result.use_perturbation() == (result.is_observable && result.is_possibly_ill_conditioned));
+    }
+
+    SUBCASE("Test use_perturbation logic directly") {


…sed-through-no-perturbation Signed-off-by: Jerry Guo <6221579+Jerry-Jinfeng-Guo@users.noreply.github.com>

sonarqubecloud · 2026-03-18T10:54:31Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

Jerry-Jinfeng-Guo added 2 commits March 6, 2026 19:42

branch init

557815f

Signed-off-by: Jerry Jinfeng Guo <jerry.jinfeng.guo@alliander.com>

add observability result and use perturbation check

a6dba93

Signed-off-by: Jerry Jinfeng Guo <jerry.jinfeng.guo@alliander.com>

Jerry-Jinfeng-Guo requested a review from Copilot March 6, 2026 19:12

Jerry-Jinfeng-Guo self-assigned this Mar 6, 2026

Jerry-Jinfeng-Guo added the feature New feature or request label Mar 6, 2026

Jerry-Jinfeng-Guo enabled auto-merge March 6, 2026 19:12

Copilot started reviewing on behalf of Jerry-Jinfeng-Guo March 6, 2026 19:12 View session

Copilot AI reviewed Mar 6, 2026

View reviewed changes

Jerry-Jinfeng-Guo and others added 5 commits March 6, 2026 20:15

Update tests/cpp_unit_tests/test_observability.cpp

0d30494

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Signed-off-by: Jerry Guo <6221579+Jerry-Jinfeng-Guo@users.noreply.github.com>

Update tests/cpp_unit_tests/test_observability.cpp

799997d

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Signed-off-by: Jerry Guo <6221579+Jerry-Jinfeng-Guo@users.noreply.github.com>

review comments

90f611d

Signed-off-by: Jerry Jinfeng Guo <jerry.jinfeng.guo@alliander.com>

notes in readme

23bdb88

Signed-off-by: Jerry Jinfeng Guo <jerry.jinfeng.guo@alliander.com>

const

33dc55d

Signed-off-by: Jerry Jinfeng Guo <jerry.jinfeng.guo@alliander.com>

figueroa1395 reviewed Mar 9, 2026

View reviewed changes

Merge branch 'main' into feature/unit-test-not-observable-network-pas…

ef87489

…sed-through-no-perturbation Signed-off-by: Jerry Guo <6221579+Jerry-Jinfeng-Guo@users.noreply.github.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Unit test not observable network passed through no perturbation#1322

Unit test not observable network passed through no perturbation#1322
Jerry-Jinfeng-Guo wants to merge 8 commits intomainfrom
feature/unit-test-not-observable-network-passed-through-no-perturbation

Jerry-Jinfeng-Guo commented Mar 6, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

figueroa1395 Mar 9, 2026

Uh oh!

figueroa1395 Mar 9, 2026

Uh oh!

figueroa1395 Mar 9, 2026

Uh oh!

figueroa1395 Mar 9, 2026

Uh oh!

figueroa1395 Mar 9, 2026

Uh oh!

figueroa1395 Mar 9, 2026

Uh oh!

figueroa1395 Mar 9, 2026

Uh oh!

figueroa1395 Mar 9, 2026

Uh oh!

sonarqubecloud bot commented Mar 18, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

Jerry-Jinfeng-Guo commented Mar 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

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!

sonarqubecloud bot commented Mar 18, 2026

Quality Gate passed

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Jerry-Jinfeng-Guo commented Mar 6, 2026 •

edited

Loading