Derive the voltage-control plan once per powerflow, in one class

[BDonnot]

Follow-up to #188 / #189: the remaining piece of the cross-solve cache, the "fancy" voltage-controller features (remote voltage control, several machines regulating one bus, SVCs).

What was there

Three derived sets described those controllers, and each was re-derived where it happened to be needed:

set	derived by
which buses a control GROUP regulates (grid ids)	LSGrid::fillpv_pq
which slack buses keep a free Vm unknown (solver ids)	Base::update_state
the controller list itself	VoltageControl::update_state, which re-ran the free-Vm slack pass of its own first

Four walks of every generator of the grid per solve, each building std::sets as it went — and three independent chances for the layers to disagree, because each walk read the containers again and nothing said they had to agree.

They were never three answers: the controller list is derived from the free-Vm slack set, which is derived from the group layout.

1. A class

src/core/VoltageControlPlan.{hpp,cpp} — one object, four layers, ~250 lines moved off LSGrid (fillpv_pq included, after review):

layer	entry point	needs
1 — group_controlled_buses()	build_groups	generators + SVCs only
2 — the pv/pq split	build_pv_pq	layer 1 + labelling + slack
3 — free_vm_slack_buses()	build_free_vm_slack	layers 1–2
4 — controllers()	build_controllers	layers 1–3

LSGrid::get_group_controlled_buses / get_free_vm_slack_solver_buses / fill_voltage_control_solver_data keep their signatures and behaviour, error messages included; they now build a throw-away plan and are documented as the on-demand form, for callers outside a solve.

2. In the cache, once per powerflow

The plan is a member of SolverBusLayout — it is expressed in that cache's bus labelling and its pv-pq split, which is why it belongs next to them rather than in the extensions that read it. A plan carried across a relabelling is not stale data, it is a different grid.

A foreign build (the batch algorithms) now publishes the plan it just built alongside the labelling, instead of leaving the extensions to re-derive one from it.

3. AlgoControl + propagation in the element modifiers

Two predicates, both on AlgoControl and both read by _build_into_cache, so what the powerflow asks and what a test asks cannot drift:

bool need_recompute_pv_pq() const noexcept {
    return need_reset_solver_ || change_dimension_ ||
           slack_participate_changed_ || pv_changed_ || pq_changed_;
}
bool need_recompute_voltage_control() const noexcept {
    return need_recompute_pv_pq() || voltage_control_changed_;
}

Since the split is layer 2 of the plan, "the split must be rebuilt" and "the plan must be rebuilt" are the same question one layer apart: who is in a control group, and which bus it regulates, are exactly the inputs the split reads. So the new tell_voltage_control_changed() carries only the one input the split does not read — a voltage setpoint, which moves no bus's pv/pq class at all — and is raised from exactly two places (GeneratorContainer::change_v_nothrow, ConverterStationContainer::change_v), on the AC family only and only for an element that actually regulates.

The review also caught three pre-existing over-invalidations, all fixed here: set_regulated_bus raising tell_recompute_sbus() (fillSbus never reads that field) and tell_pv_changed() unconditionally, and the converter station's _deactivate / _reactivate raising tell_pv_changed() for non-regulating stations.

4. An algorithm that cannot do voltage control now says so

BaseAlgo::supports_remote_voltage_control() existed and had no caller. Fast-decoupled and Gauss-Seidel hold no NRSystem, so on a grid with control groups they converged 0.36 pu away from the Newton-Raphson answer, missing setpoints by 0.127 pu — silently. ac_pf now refuses such a grid with the full list of offending generators, SVCs and stations; for an algorithm that cannot, no plan is built at all (layer 2 produces the classical split, layers 1/3/4 are skipped), which also stops it paying for work it never reads.

Two smaller fixes found on the way: change_algorithm(const std::string&) never called init_fdpf_coeffs(), and the profiling driver's MAX_ITER = 10 diverges for FDPF (~50 needed) — it now derives the budget from BaseAlgo::is_fdpf().

Measurements

callgrind, KLU, -O3, tol = 1e-8; every solve's iteration count and full complex voltage vector compared between the two builds — all traces bit-identical, on every phase.

grid	inj (an ordinary step)	idem (the floor)	topo (a line toggled)
case30	-4.9%	-7.9%	-2.3%
case118	-8.3%	-16.9%	-4.9%
case118_fancy	-7.0%	-18.6%	-4.0%
case1354pegase	-3.2%	-7.3%	-1.9%
case1354pegase_fancy	-2.6%	-3.9%	-1.7%
case9241pegase	-2.2%	-5.4%	-1.4%
case9241pegase_fancy	-0.4%	-0.8%	-0.5%

idem and inj save the same number of instructions to the last one: what is removed is a fixed per-solve cost, before the Newton loop starts. inj reads smaller only because it is a 1.5x–2.7x bigger solve. About 1.9k of the figure is an unrelated find: AlgorithmSelector took its error_msg by const std::string &, and every call site passes a literal longer than libstdc++'s SSO buffer — a malloc and a free per solve for a string only the error path reads.

Part of the topo column is the term the review disputed. need_recompute_pv_pq() listed ybus_change_sparsity_pattern_; the reviewer disagreed and asked for a test rather than an argument, so the [pv_pq] cases in test_cache_reuse.cpp reach a state where one term is raised and the other five are not, solve warm, reset, solve cold and compare — and the predicate was then built both ways:

dropping ybus_change_sparsity_pattern_  ->  41 assertions, all pass   -> dropped
dropping slack_participate_changed_     ->  1 of 2 cases fails        -> kept

Worth -0.9% / -1.8% / -0.7% / -0.4% on its own (case30 / case118 / case1354pegase / case9241pegase, topo, same tree with the term restored).

make_grids.py also writes _fancy variants of case118 / case1354pegase / case9241pegase — pairs of generators re-pointed at a common neighbouring load bus plus voltage-mode SVCs. The plain pandapower cases have no remote control and no SVC at all, so nothing about the bordered block showed up in the #190 audit.

Two findings, neither in scope

• On case9241pegase_fancy, 86% of a cached solve is klu_refactor (55% on the plain case). Forty two-member groups add 80 reactive-injection columns, each coupling a generator's bus to a regulated bus a branch away, and KLU's ordering pays the fill-in.
• The singular configurations hit while writing the tests (a local PV regulator co-existing with a remote group) reproduce identically on the pre-change binary. That is the open TODO entry, not a regression.

Tests

• C++: 249 test cases / 591,174 assertions green, including the new src/tests/test_voltage_control_plan_cache.cpp (every input of the plan mutated on a grid that has already solved, so a stale plan is available to be wrongly reused, and on a fresh grid built in the final state — a reused stale plan does not throw and does not look wrong, it converges on the previous scenario's controllers) and the new [pv_pq] section of test_cache_reuse.cpp.
• Python: 188 passed / 65 skipped across the tests touching this area. The full suite's remaining failures are all missing grid2op data_test fixtures in this container and reproduce without the change.

🤖 Generated with Claude Code

https://claude.ai/code/session_01Qz1LmyVeo1nnhrJvLJv9iY

---

Generated by Claude Code

[BDonnot]

Lots of changes to make and things that are unclear

Also some tests might need to be added for the flag raised in the grid modifiers.

[BDonnot]

Still don't get why no HVDC here. An already present bug ?

[BDonnot]

Same answer as on the .cpp — see that thread. Short version: not a bug. A station has no regulated bus of its own (the container stores none), so it can only ever regulate the bus it stands on, through the ordinary PV path — it can never create a group, only join one somebody else created, which build_controllers does. Both ends now say so explicitly in 7ec0ba3, and "remote regulation by a station is not modelled" is now a changelog TODO.

---

Generated by Claude Code

[BDonnot]

Why ? Only value changed in this case. And value is already tagged above

[BDonnot]

This one I kept, but your objection made me narrow it and it is now the only reason the flag still exists.

tell_v_changed() above is not enough because v_changed_ is no longer part of need_recompute_voltage_control() — you were right that it did not belong in that union (see the Utils.hpp thread). And a setpoint really is the one input of the plan that nothing else carries: moving a remote regulator's target changes no bus' pv/pq class at all — that is the point of the bordered formulation, the regulated bus stays PQ — so pv_changed_ never fires and the plan would be reused with a stale v_set.

Narrowed to only fire when the station actually regulates, since target_vm_pu_ of one that does not is never read:

if(voltage_regulator_on_[station_id]) solver_control.ac_algo_controler().tell_voltage_control_changed();

There is a test for it, and it fails if the term is dropped from the predicate.

---

Generated by Claude Code

[BDonnot]

Review answered in 7ec0ba3

You were right that moving fillpv_pq into the plan changes the answers — but mostly by making your objections more right, not less. The pv/pq split is now layer 2 of the plan, so "the split must be rebuilt" and "the plan must be rebuilt" are the same question asked one layer apart, and the flags I had been listing were already carried by pv_changed_ / slack_participate_changed_.

The predicate. Two now, both on AlgoControl, and the second is visibly the first plus one term:

bool need_recompute_pv_pq() const noexcept {
    return need_reset_solver_ || change_dimension_ || ybus_change_sparsity_pattern_ ||
           slack_participate_changed_ || pv_changed_ || pq_changed_;
}
bool need_recompute_voltage_control() const noexcept {
    return need_recompute_pv_pq() || voltage_control_changed_;
}

slack_weight_changed_, one_el_change_bus_ and v_changed_ are gone. slack_participate_changed_ and ybus_change_sparsity_pattern_ stay, but as pv/pq terms — the slack is not PV, and the sparsity flag is what re-labels the buses — which is a reason you can check rather than a claim about voltage control. _build_into_cache reads those two and nothing else, so what the powerflow asks and what a test asks cannot drift.

The propagation: 15 call sites → 2. tell_voltage_control_changed() now means one thing only — a voltage setpoint moved — and lives in GeneratorContainer::change_v_nothrow and ConverterStationContainer::change_v, gated on voltage_regulator_on_. That is the one input the split does not read: moving a remote regulator's target changes no bus' class at all. Everywhere else you flagged (_deactivate, _reactivate, _change_bus, _change_p, add_slackbus, remove_slackbus, turnedoff_*, set_regulated_bus, the three SVC sites) the line is deleted, because the tell_pv_changed() / tell_slack_participate_changed() already there does the job.

Three flags were wrong before this branch, and your comments found all three:

where	was	now
GeneratorContainer::set_regulated_bus	tell_recompute_sbus()	removed — fillSbus never reads regulated_bus_id_, and what it stamps for a regulating gen (active power only) is the same wherever it regulates
GeneratorContainer::set_regulated_bus	tell_pv_changed() unconditional	gated on voltage_regulator_on_ — the field is inert otherwise
ConverterStationContainer::_deactivate / _reactivate	tell_pv_changed() for every station	gated on voltage_regulator_on_ — fillpv skips the others

Are stations in Sbus? Yes — fillSbus_station stamps every connected one (active power always, reactive too when it does not regulate). So tell_recompute_sbus() there is right and stays; the line is now commented to say so.

Why no HVDC in build_groups? Not a bug. A station has no regulated bus of its own — the container stores none, a regulating station always regulates the bus it stands on — so it can never be the remote controller that creates a group. It pins its own bus through fillpv like a local generator, and joins a group only when one already claims that bus, which is build_controllers' job and is keyed on what build_groups returns. Spelled out at both ends now.

TODOs added (top of the changelog) for the four inputs with no runtime setter at all, so a caller cannot change them on a live grid: a generator's and a station's voltage_regulator_on_, an SVC's regulation_mode_ and regulated_bus_id_, and remote regulation by a station, which is not modelled. Each says what flag the setter must raise.

Left open deliberately — four threads where I did not do what was asked and you should judge: the two change_v sites (I kept the flag, narrowed; it is the only non-redundant use left) and the two HVDC-in-build_groups ones (I answered rather than changed code).

Checks. C++ 247 cases / 591,133 assertions green; targeted python 245 passed / 65 skipped; full python suite 1440 passed, the 99 failures all missing grid2op data_test fixtures in this container and identical on the base. Instruction counts unchanged and all 14 (grid, phase) traces still bit-identical to the pre-PR build. Both invalidation paths mutation-checked: dropping the setpoint term, and silencing set_regulated_bus, each make a specific test fail.

---

Generated by Claude Code

[BDonnot]

A few tests to do

[BDonnot]

Disagree with slack_partcipate_changed, make another test

[BDonnot]

Same experiment, opposite verdict: this one is load-bearing, and the test now proves it rather than asserting it. slack_participate_changed_ stays (18988f9).

TEST_CASE("slack_participate_changed, on its own, DOES move the split", "[LSGrid][cache_reuse][pv_pq]") — two sections, the slack moved to another generator (update_slack_weights_by_id) and a second generator joining it (add_gen_slackbus). Each asserts the term is raised and the other five are not, then solves warm, resets, solves cold and compares. With the term dropped from the predicate:

  the slack moves to another generator
  a second generator joins the slack
test cases:  2 |  1 passed | 1 failed
assertions: 37 | 35 passed | 2 failed

And the reason has nothing to do with voltage, which is where I think our disagreement was — you are right that a non-reference slack has nothing to do with voltage. It is that the split reads the slack set directly:

// GeneratorContainer::fillpv
if(is_in_vect(bus_id_solver.cast_int(), slack_bus_id_solver.to_int_vector())) continue;  // slack bus is not PV

and the PQ loop right after it skips slack buses too. So a bus joining the slack set must leave bus_pv, and one leaving it must join bus_pv or bus_pq. Move the slack set and the split moves with it, whatever the machine does to voltage.

---

Generated by Claude Code

[BDonnot]

Ok