Repository navigation
Review fixes for #222: slack overshoot, VSC limits, OLF voltage range, sharing-key cost - #223
Merged
Merged
Conversation
distribute_with_overshoot only reported "all saturated" when every participant ran out of room. A unit only flagged "can participate" with room left kept that false, every unit of the Newton solve's slack was marked saturated, and the caller took them all out of it: the next solve had an empty distributed slack. Apply the rule `distribute` already applies: when every in_slack unit hit its bound, clear the mask and set all_saturated. Assisted-by: Claude Code Claude-Session: https://claude.ai/code/session_01GbSAUSwx459CCq4pn9oTkD Signed-off-by: Benjamin Donnot <benjamin.donnot@rte-france.com>
The overshoot was a magnitude, and distribute_with_overshoot guessed its side from where the unit sits. A charging storage unit a positive mismatch capped at 0 MW sits at 0, which the sign rule reads as the LOWER bound of an injecting unit: the overshoot was put below 0, a later negative mismatch could not move the unit at all, and a positive one moved it above 0 once a phantom overshoot was used up. The overshoot is now signed: > 0 beyond the upper limit, < 0 beyond the lower one. The bake returns it so, the setter accepts any finite value, and at 0 MW the sign tells which side of 0 the unit is on. Assisted-by: Claude Code Claude-Session: https://claude.ai/code/session_01GbSAUSwx459CCq4pn9oTkD Signed-off-by: Benjamin Donnot <benjamin.donnot@rte-france.com>
The swap of inverted min_q / max_q writes into the arrays `Series.to_numpy(float)` returns, which pandas copy-on-write (the default from pandas 3) hands out read-only: init_from_pypowsybl raised "assignment destination is read-only" on every network with an HVDC line, swapped limits or not (an empty boolean mask still writes). Assisted-by: Claude Code Claude-Session: https://claude.ai/code/session_01GbSAUSwx459CCq4pn9oTkD Signed-off-by: Benjamin Donnot <benjamin.donnot@rte-france.com>
`can_participate_slack_overshoot` was only read inside the branch that applies `can_participate_slack`: given alone, or with an explicit slack, it was dropped silently and every capped unit left its limit at once. Raise, as `can_participate_slack` itself does in the same situation. Assisted-by: Claude Code Claude-Session: https://claude.ai/code/session_01GbSAUSwx459CCq4pn9oTkD Signed-off-by: Benjamin Donnot <benjamin.donnot@rte-france.com>
The other bake flags (can_be_pv, the "can participate in the slack" weight and overshoot) are compared; this one was not, so two grids differing only by which hvdc lines are frozen compared equal and a regression in its binary round trip or its propagation went unseen. Assisted-by: Claude Code Claude-Session: https://claude.ai/code/session_01GbSAUSwx459CCq4pn9oTkD Signed-off-by: Benjamin Donnot <benjamin.donnot@rte-france.com>
The VSC converter read duplicated the generators' one (the curve at the target P when the flat box is NaN, the sort of an inverted pair), and the two copies had already drifted apart: one wrote into a read-only array under pandas copy-on-write. Both now go through _aux_reactive_limits_at_target_p; each keeps its own bound for NaN. No behaviour change: the limits init_from_pypowsybl reads are bit-identical before and after on ieee14, ieee118, the four-substations network and on CURVE / swapped-curve / HVDC variants. Assisted-by: Claude Code Claude-Session: https://claude.ai/code/session_01GbSAUSwx459CCq4pn9oTkD Signed-off-by: Benjamin Donnot <benjamin.donnot@rte-france.com>
The bake rebuilt the main-component filter by hand (connected and synchronous component 0 on both stations) next to the module's own _keep_only_main_comp: two definitions that would drift apart the day the rule changes. Filter the stations through it instead. No behaviour change: the lines baked, their target_p and converters mode are identical before and after on the hvdc test networks (saturated, linear, operator-range, a station disconnected), with and without keep_only_main_comp. Assisted-by: Claude Code Claude-Session: https://claude.ai/code/session_01GbSAUSwx459CCq4pn9oTkD Signed-off-by: Benjamin Donnot <benjamin.donnot@rte-france.com>
sharing_weight recomputed, for every controller, the share of the bus of each of its peers, each by a pass over all the members of the group: cubic in the size of a group, at every rebuild of the plan. What it sums does not depend on the controller beyond its own membership, so the per-bus shares, whether the buses of the active members are all keyed, and the sums inside each bus are computed once per group; a held controller adds its own bus and terms on top, as its peer list did. The sums are accumulated in the same order: the weights are bit-identical before and after (checked on 1335 controllers over mixed keyed / unkeyed buses, held members and passive units), and a group of 2000 controllers on 400 buses builds in about a millisecond instead of several seconds. Assisted-by: Claude Code Claude-Session: https://claude.ai/code/session_01GbSAUSwx459CCq4pn9oTkD Signed-off-by: Benjamin Donnot <benjamin.donnot@rte-france.com>
init_from_pypowsybl(remote_voltage_control_vm_range="olf") used a hard-coded minRealisticVoltage / maxRealisticVoltage of 0.8 / 1.2, but OpenLoadFlow's defaults differ between versions: the one pypowsybl 1.16.1 ships uses 0.5 / 2.0. There, lightsim2grid reported remote controllers OpenLoadFlow never switches, and test_reported_where_olf_switches failed in CI. "olf" now reads both defaults off the installed OpenLoadFlow (get_provider_parameters), with the margin of its loop; the constants are only the fallback when pypowsybl does not expose them. The two tests that check the reporting itself, not the default, pass an explicit range. Assisted-by: Claude Code Claude-Session: https://claude.ai/code/session_01GbSAUSwx459CCq4pn9oTkD Signed-off-by: Benjamin Donnot <benjamin.donnot@rte-france.com>
BDonnot
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
These are fixes for problems a review of #222 found, plus the one test that fails in #222's CI. The PR targets
hvdc_operator_range, #222's own branch, so merging it updates #222. There is one commit per fix. For every bug, a test was written first and failed on9d954e2, the head of #222, then passed once the fix was in.Bugs
c03e54d).distribute_with_overshootnever cleared the saturated flags when every unit of the solve's slack hit its bound while a unit that may only "participate" still had room. All the slack units then left the slack, and the next solve had none. It now handles this case asdistribute()already does.1b41674). The overshoot was a magnitude. A charging battery capped at 0 MW from below was read as sitting at its lower bound, so a negative mismatch could not move it and a positive one pushed it above 0. The overshoot is now signed: positive means above the upper limit, negative means below the lower one. The bake returns the sign and the setter accepts negative values.8309c21). The swap of the VSC reactive limits wrote into read-only arrays under pandas copy-on-write, which is the default from pandas 3. As a result,init_from_pypowsyblraised on every network with an HVDC line.d13b6cc).can_participate_slack_overshootwithoutcan_participate_slackwas ignored. It now raises, ascan_participate_slackalready does in the same situation.compare_lsgridmissed a field (2e0c17e): it now compares the HVDCac_emulation_frozenflag.e795d72).remote_voltage_control_vm_range="olf"used a hard-coded 0.8 / 1.2, but OLF's defaults differ between versions: the OLF in pypowsybl 1.16.1 uses 0.5 / 2.0. With that version, lightsim2grid reported remote controllers that OLF never switches, which is whytest_reported_where_olf_switchesfailed in Match OpenLoadFlow on hvdc limits, VSC curves, SVC standby b0 and remote voltage control #222's CI."olf"now readsminRealisticVoltage/maxRealisticVoltagefrom OLF's provider parameters. The constants are only a fallback when pypowsybl doesn't expose them.No behaviour change (outputs checked identical before and after)
d4a8a9e). Generators and VSC stations now read their capability curve through one shared helper. The limits are bit-identical on 8 networks.0cdfb6d). The HVDC AC-emulation bake now uses_keep_only_main_compinstead of its own copy of that filter. Its output is identical on 6 cases.d9f62d0). The cost per group goes from cubic to linear. The weights are bit-identical on 1335 controllers. A group of 2000 controllers now builds in about 1 ms instead of 4.6 s.Left out
Testing
LSGrid.cpp,VoltageControlPlan.cppandGeneratorContainer.cpppass a C++14 syntax check.test_remote_voltage_control,test_can_participate_slack,test_hvdc_pypowsybl,test_olf_bake,test_init_from_pypowsybl,test_LSGrid_pypowsybl,test_voltage_control_pypowsybl,test_gen_reactive_curve_swap,test_binary_serialization,test_deepcopy, the batch slack-redistribution and main-component files, and the voltage-control files all pass.🤖 Generated with Claude Code
https://claude.ai/code/session_01GbSAUSwx459CCq4pn9oTkD