Skip to content

Unit test not observable network passed through no perturbation #1322

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
Jerry-Jinfeng-Guo wants to merge 8 commits into
base: main
Choose a base branch
from
+170 −0
Open

Unit test not observable network passed through no perturbation #1322

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
557815f
branch init
Jerry-Jinfeng-Guo Mar 6, 2026
a6dba93
add observability result and use perturbation check
Jerry-Jinfeng-Guo Mar 6, 2026
0d30494
Update tests/cpp_unit_tests/test_observability.cpp
Jerry-Jinfeng-Guo Mar 6, 2026
799997d
Update tests/cpp_unit_tests/test_observability.cpp
Jerry-Jinfeng-Guo Mar 6, 2026
90f611d
review comments
Jerry-Jinfeng-Guo Mar 6, 2026
23bdb88
notes in readme
Jerry-Jinfeng-Guo Mar 6, 2026
33dc55d
const
Jerry-Jinfeng-Guo Mar 9, 2026
ef87489
Merge branch 'main' into feature/unit-test-not-observable-network-pas…
Jerry-Jinfeng-Guo Mar 18, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
137 changes: 137 additions & 0 deletions tests/cpp_unit_tests/test_observability.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2012,4 +2012,141 @@ TEST_CASE("Test Observability - Necessary check end to end test") {
}
}

TEST_CASE("Test ObservabilityResult - use_perturbation with non-observable network") {
using power_grid_model::math_solver::observability::ObservabilityResult;

SUBCASE("Non-observable network returns false for use_perturbation") {
// Create a meshed network with multiple voltage phasor sensors
// This triggers the early return for the condition n_voltage_phasor_sensors > 1 && !topo.is_radial,
// where is_observable = false but no exception is thrown
MathModelTopology topo;
topo.slack_bus = 0;
topo.is_radial = false; // Meshed network
topo.phase_shift = {0.0, 0.0, 0.0, 0.0};
// Create a meshed network: bus0--bus1, bus1--bus2, bus2--bus3, bus3--bus0
topo.branch_bus_idx = {{0, 1}, {1, 2}, {2, 3}, {3, 0}};
topo.sources_per_bus = {from_sparse, {0, 1, 1, 1, 1}};
topo.shunts_per_bus = {from_sparse, {0, 0, 0, 0, 0}};
topo.load_gens_per_bus = {from_sparse, {0, 0, 0, 0, 0}};
topo.power_sensors_per_bus = {from_sparse, {0, 0, 0, 0, 0}};
topo.power_sensors_per_source = {from_sparse, {0, 0}};
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}};
Copy link
Member

@figueroa1395 figueroa1395 Mar 9, 2026

Choose a reason for hiding this comment

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

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

topo.power_sensors_per_branch_to = {from_sparse, {0, 0, 0, 0, 0}};
topo.current_sensors_per_branch_from = {from_sparse, {0, 0, 0, 0, 0}};
topo.current_sensors_per_branch_to = {from_sparse, {0, 0, 0, 0, 0}};
// TWO voltage phasor sensors (complex measurements with both magnitude and angle)
// This triggers the condition: n_voltage_phasor_sensors > 1 && !topo.is_radial
topo.voltage_sensors_per_bus = {from_sparse, {0, 1, 2, 2, 2}};

MathModelParam<symmetric_t> param;
param.source_param = {SourceCalcParam{.y1 = 1.0, .y0 = 1.0}};
param.branch_param = {
{1.0, -1.0, -1.0, 1.0}, {1.0, -1.0, -1.0, 1.0}, {1.0, -1.0, -1.0, 1.0}, {1.0, -1.0, -1.0, 1.0}};

StateEstimationInput<symmetric_t> se_input;
se_input.source_status = {1};
// Two voltage PHASOR sensors (complex with both magnitude and angle)
se_input.measured_voltage = {
{.value = 1.0 + 0.1i, .variance = 1.0}, // Phasor at bus 0
{.value = 0.95 + 0.05i, .variance = 1.0} // Phasor at bus 1
};
// Branch power measurements
se_input.measured_branch_from_power = {
{.real_component = {.value = 1.0, .variance = 1.0}, .imag_component = {.value = 0.0, .variance = 1.0}},
{.real_component = {.value = 1.0, .variance = 1.0}, .imag_component = {.value = 0.0, .variance = 1.0}},
{.real_component = {.value = 1.0, .variance = 1.0}, .imag_component = {.value = 0.0, .variance = 1.0}},
{.real_component = {.value = 1.0, .variance = 1.0}, .imag_component = {.value = 0.0, .variance = 1.0}}};

auto topo_ptr = std::make_shared<MathModelTopology const>(topo);
auto param_ptr = std::make_shared<MathModelParam<symmetric_t> const>(param);
YBus<symmetric_t> const y_bus{topo_ptr, param_ptr};
math_solver::MeasuredValues<symmetric_t> const measured_values{*y_bus.shared_topology(), se_input};

// Verify that we have exactly 2 voltage phasor sensors (both with angle measurements)
Idx phasor_count = 0;
for (Idx bus = 0; bus < topo.n_bus(); ++bus) {
if (measured_values.has_voltage(bus) && measured_values.has_angle_measurement(bus)) {
++phasor_count;
}
}
CHECK(phasor_count == 2); // Verify we have 2 voltage phasor sensors

// Get the observability result - should return is_observable = false without throwing
// because of early return: n_voltage_phasor_sensors > 1 && !topo.is_radial
auto result = math_solver::observability::observability_check(measured_values, y_bus.math_topology(),
y_bus.y_bus_structure());

// Verify that is_observable is false (due to multiple voltage phasors in meshed network)
CHECK(result.is_observable == false);

// Verify that use_perturbation() returns false when not observable
CHECK(result.use_perturbation() == false);
Comment on lines +2083 to +2086
Copy link
Member

@figueroa1395 figueroa1395 Mar 9, 2026

Choose a reason for hiding this comment

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

I think this behavioral check is good and valuable.

}

SUBCASE("Observable but ill-conditioned network returns true for use_perturbation") {
// Create a simple 3-bus network with sufficient sensors (observable but possibly ill-conditioned)
MathModelTopology topo;
topo.slack_bus = 0;
topo.is_radial = true;
topo.phase_shift = {0.0, 0.0, 0.0};
topo.branch_bus_idx = {{0, 1}, {1, 2}};
topo.sources_per_bus = {from_sparse, {0, 1, 1, 1}};
topo.shunts_per_bus = {from_sparse, {0, 0, 0, 0}};
topo.load_gens_per_bus = {from_sparse, {0, 0, 0, 0}};
topo.power_sensors_per_bus = {from_sparse, {0, 0, 0, 0}};
topo.power_sensors_per_source = {from_sparse, {0, 0}};
topo.power_sensors_per_load_gen = {from_sparse, {0}};
topo.power_sensors_per_shunt = {from_sparse, {0}};
// Add branch sensors to make it observable
topo.power_sensors_per_branch_from = {from_sparse, {0, 1, 2}};
topo.power_sensors_per_branch_to = {from_sparse, {0, 0, 0}};
topo.current_sensors_per_branch_from = {from_sparse, {0, 0, 0}};
topo.current_sensors_per_branch_to = {from_sparse, {0, 0, 0}};
topo.voltage_sensors_per_bus = {from_sparse, {0, 1, 1, 1}};

MathModelParam<symmetric_t> param;
param.source_param = {SourceCalcParam{.y1 = 1.0, .y0 = 1.0}};
param.branch_param = {{1.0, -1.0, -1.0, 1.0}, {1.0, -1.0, -1.0, 1.0}};

StateEstimationInput<symmetric_t> se_input;
se_input.source_status = {1};
se_input.measured_voltage = {{.value = 1.0, .variance = 1.0}};
se_input.measured_branch_from_power = {
{.real_component = {.value = 1.0, .variance = 1.0}, .imag_component = {.value = 0.0, .variance = 1.0}},
{.real_component = {.value = 1.0, .variance = 1.0}, .imag_component = {.value = 0.0, .variance = 1.0}}};

auto topo_ptr = std::make_shared<MathModelTopology const>(topo);
auto param_ptr = std::make_shared<MathModelParam<symmetric_t> const>(param);
YBus<symmetric_t> const y_bus{topo_ptr, param_ptr};
math_solver::MeasuredValues<symmetric_t> const measured_values{*y_bus.shared_topology(), se_input};

auto result = math_solver::observability::observability_check(measured_values, y_bus.math_topology(),
y_bus.y_bus_structure());

// Verify that is_observable is true
CHECK(result.is_observable == true);

// use_perturbation() should follow the invariant: is_observable && is_possibly_ill_conditioned
CHECK(result.use_perturbation() == (result.is_observable && result.is_possibly_ill_conditioned));
}

SUBCASE("Test use_perturbation logic directly") {
Copy link
Member

@figueroa1395 figueroa1395 Mar 9, 2026

Choose a reason for hiding this comment

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

Nice!

// Test the logic of use_perturbation() method directly
const ObservabilityResult result1{.is_observable = false, .is_possibly_ill_conditioned = false};
CHECK(result1.use_perturbation() == false);

const ObservabilityResult result2{.is_observable = false, .is_possibly_ill_conditioned = true};
CHECK(result2.use_perturbation() == false);

const ObservabilityResult result3{.is_observable = true, .is_possibly_ill_conditioned = false};
CHECK(result3.use_perturbation() == false);

const ObservabilityResult result4{.is_observable = true, .is_possibly_ill_conditioned = true};
CHECK(result4.use_perturbation() == true);
}
}

} // namespace power_grid_model
33 changes: 33 additions & 0 deletions tests/data/state_estimation/meshed-network-observability/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,3 +33,36 @@ Observable system with 2 voltage sensors.

Note: In theory, it is impossible to construct an unobservable case with only nodal measurements
(i.e., without branch measurement).

## Multiple Voltage Phasor Sensors in Meshed Networks

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.
Comment on lines +39 to +42
Copy link
Member

@figueroa1395 figueroa1395 Mar 9, 2026

Choose a reason for hiding this comment

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

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.


### Unit Test Coverage

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
Comment on lines +46 to +53
Copy link
Member

@figueroa1395 figueroa1395 Mar 9, 2026

Choose a reason for hiding this comment

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

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


### Why No "Expected to Fail" Integration Test?

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.
Comment on lines +57 to +64
Copy link
Member

@figueroa1395 figueroa1395 Mar 9, 2026

Choose a reason for hiding this comment

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

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.


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.
Comment on lines +66 to +68
Copy link
Member

@figueroa1395 figueroa1395 Mar 9, 2026

Choose a reason for hiding this comment

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

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).

Loading