Repository navigation
Fixes from the review of #226: topological actions in ScenarioSweep, LightEnv - #228
Merged
Merged
Conversation
`_line_mask`, `_trafo_mask` and `_has_topo_actions` were written and reset in five places and read nowhere: run() takes each row's disconnected branches from C++ (get_row_disconnected_branches), which merges the masks and the topological action itself. The comments saying run() needs the cache went with it, and run()'s docstring now says where element_ids come from. Assisted-by: Claude Code Claude-Session: https://claude.ai/code/session_014PrXNr1rwq4PxLAZ9pL3yu Signed-off-by: Benjamin Donnot <benjamin.donnot@rte-france.com>
_reset_topo_policy_state() cleared the branches a row's action takes out but not the ones it places (topo_branches_moved). A batch computed after clear() with no action registered returns from _maybe_resolve_topology before anything is resolved, so init_li_coeffs_from_masks and _prepare_connectivity replayed the old batch's placements, with solver ids of the old union layout. Reproduced with a branch reconnected by an action, then clear() and a plain batch on the same grid: the second compute writes admittance entries the new pattern does not hold, a row fails, and the process aborts on a corrupted heap (munmap_chunk(): invalid pointer). The placements are now cleared with the rest; the new test compares that batch with a fresh sweep. Assisted-by: Claude Code Claude-Session: https://claude.ai/code/session_014PrXNr1rwq4PxLAZ9pL3yu Signed-off-by: Benjamin Donnot <benjamin.donnot@rte-france.com>
get_row_disconnected_branches refused to answer while the batch inputs were not valid, and that flag is only raised once the "n" powerflow has converged. ScenarioSweep.run() calls it for every row after compute(ignore_errors=True), so with topological actions registered a diverging "n" case raised "call compute first" instead of being reported through its sentinel violations, which is what run() documents. The actions are resolved before the "n" solve. A flag of their own (_topo_resolved_) says so: raised at the end of _maybe_resolve_topology, dropped with the batch inputs, unconditionally (a batch whose "n" case diverged was never valid, its resolution still stands until the inputs change). Assisted-by: Claude Code Claude-Session: https://claude.ai/code/session_014PrXNr1rwq4PxLAZ9pL3yu Signed-off-by: Benjamin Donnot <benjamin.donnot@rte-france.com>
…n" case _record_n_case_violations passed no masked list to the bus voltage check. With topological actions, the union layout holds busbars no element of the base grid stands on; the "n" solve masks them, so they read the seed they were given (busbar 1's starting voltage), not a solved one, and get_violations_n() could report a violation on a bus the base grid does not have. A row that does not use them already skips them. The base mask (_base_masked_) is now passed, as the rows pass theirs. The physical checks of the "n" case need nothing: their plans only list buses a controller of the base grid stands on. Assisted-by: Claude Code Claude-Session: https://claude.ai/code/session_014PrXNr1rwq4PxLAZ9pL3yu Signed-off-by: Benjamin Donnot <benjamin.donnot@rte-france.com>
A move leaves actions_ and init_grid_ null, and nb_actions(), get_actions() and init_actions() dereferenced them unchecked, where the class documents that a moved-from env throws std::logic_error on anything but destruction or assignment. They now run the same check as reset() and step(). The moved-from helper of the C++ tests asks the three of them; before this change nb_actions() returned instead of throwing. Assisted-by: Claude Code Claude-Session: https://claude.ai/code/session_014PrXNr1rwq4PxLAZ9pL3yu Signed-off-by: Benjamin Donnot <benjamin.donnot@rte-france.com>
Protections::update_rho assigned its timer (=) where the other timers add up (+=), and reset_timers() left it out, so update_rho_time held the duration of the last call only, while the binding documents it as cumulated over an episode. It now adds up and is reset with the others. The test steps the env twenty times: the readings must never decrease, and a reset must bring the value back down. Before the change they went up and down from one step to the next. Assisted-by: Claude Code Claude-Session: https://claude.ai/code/session_014PrXNr1rwq4PxLAZ9pL3yu Signed-off-by: Benjamin Donnot <benjamin.donnot@rte-france.com>
A branch on in the base grid with one end open was handled as if both ends were closed. A row moving its closed end put the closed-branch block on both buses, the open end included. A row reconnecting it with set_line_status was read as a no-op (same global status, same buses), while TopoAction::apply_to_gridmodel closes the open end on the bus it was last on. Reproduced on case14 with one end of line 5 opened: the reconnecting row came out with voltages about 0.02 pu away from the one-off powerflow, and both rows were solved without a word. What a row should do with that open end is not settled, so a row that puts such a branch on is now refused, by name. resolve_row_topo lists it even when the buses do not change, so that the refusal sees it. Disconnecting one stays allowed: the coefficients taken out are the branch's effective block, and the rows match the reference. A branch reconnected to an end with no bus recorded is refused as well, where its -1 used to reach the union layout and the admittance edits. Assisted-by: Claude Code Claude-Session: https://claude.ai/code/session_014PrXNr1rwq4PxLAZ9pL3yu Signed-off-by: Benjamin Donnot <benjamin.donnot@rte-france.com>
Without handle_disconnected_grid, a row is skipped when it masks a bus other than the union layout's extra busbars. A busbar the row's action leaves with no element at all fell on the wrong side of that rule: a merge (every element of busbar 2 moved back to busbar 1, on a grid that splits the substation) was NOT_SIMULATED, though nothing is islanded and nothing lost, while a load left alone on an extra busbar, which does lose its injection, was solved. _maybe_resolve_topology now lists, per row, the base buses its action empties (the base grid's per-bus element counts, with each element the plan takes off a bus or puts on one moved accordingly; a branch end counts where it is closed), and the connectivity verdict and the skip mask accept those as masked by design. Only the action's own moves count: a row without one keeps the answer the masks always gave. test_row_splitting_the_grid took generator 4 out with trafo 3, so its row emptied the bus rather than stranding it. It now has both rows: trafo 3 alone (generator 4 islanded: NOT_SIMULATED by default) and the two together (solved in either mode). The slack pre-pass tests no longer turn handle_disconnected_grid on: they needed it for that rule only. Assisted-by: Claude Code Claude-Session: https://claude.ai/code/session_014PrXNr1rwq4PxLAZ9pL3yu Signed-off-by: Benjamin Donnot <benjamin.donnot@rte-france.com>
With modify_gen_v, the buses several generators regulate are found once per compute(), from the base placement. A row that moves a generator off such a bus was then held to its old group. Reproduced on case14 with generator 3 moved off the bus it shares with generator 2, its own set-point raised by 0.02 pu. _row_gen_v_conflicts compared it with generator 2 and skipped the row (NOT_SIMULATED). With the comparison fixed alone, set_vm (base placement, last writer wins) still seeded the shared bus with the set-point of the generator that left, and the row came out about 0.02 pu off the one-off powerflow. A row's generators that are off their base bus (moved, or disconnected: the gen_off it already carries) are now left out of the comparison, and after set_vm a shared bus a row takes a generator off is set back to the set-point of what stays there: the generators left, or the element no row moves. Each constraint keeps its bus for that. The new test fails without either half. Assisted-by: Claude Code Claude-Session: https://claude.ai/code/session_014PrXNr1rwq4PxLAZ9pL3yu Signed-off-by: Benjamin Donnot <benjamin.donnot@rte-france.com>
The PV -> PQ handling of a row only counted generators as the controllers of a bus. A row taking out a storage unit that regulates its bus left that bus PV, held at the set-point of a unit that is gone; a row taking out every generator of a bus a storage unit also regulates turned it PQ while the unit still held it. Reproduced on educ_case14_storage with two regulating storage units: about 0.03 and 0.05 pu off the one-off powerflow, both rows reported converged. _maybe_prepare_gen_contingency now lists the regulating storage units per bus next to the generators, and a bus flips once a row takes out all of them. A row only ever takes such a unit out: placing one is refused. The bus reactive check (BusQCheck) leaves out a storage unit the row disconnects, as it does a generator; the solves that disconnect none keep the previous signature. Each half has a test that fails without it. Assisted-by: Claude Code Claude-Session: https://claude.ai/code/session_014PrXNr1rwq4PxLAZ9pL3yu Signed-off-by: Benjamin Donnot <benjamin.donnot@rte-france.com>
A row's distributed-slack weights were only re-derived for the generators it disconnects: a storage unit with a slack weight kept its share on its base bus whatever the row's action did with it. Reproduced on educ_case14_storage with storage 0 in the slack: the row disconnecting it put part of the imbalance on a bus with nothing to absorb it, the row moving it put its share on the bus it left (voltages up to 0.03 pu off the one-off powerflow). The two cases are now handled as for a generator. Taken out, the unit is left out of the row's weights (_row_slack_storages_off_, read by _row_slack_weights through get_slack_weights_solver_without, with the refactorization fallback that goes with weights that vary per row). Moved or reactivated, it would take its share on another bus and change the slack set with the row: refused, by name. Assisted-by: Claude Code Claude-Session: https://claude.ai/code/session_014PrXNr1rwq4PxLAZ9pL3yu Signed-off-by: Benjamin Donnot <benjamin.donnot@rte-france.com>
_prepare_connectivity answers each row from one DFS of the base graph (BusGraph) and falls back to a breadth-first search of a patched matrix copy when the tree cannot settle it, a base graph that is not connected included. The union layout of a batch with topological actions holds busbars no element of the base grid stands on, isolated in the base matrix: once a single action created a bus, every row went to the search, plain rows included, which on a large grid turns the per-row check from constant time into a pass over the whole matrix. BusGraph::build takes the buses known to be isolated and leaves them out of the tree (rooted at the first bus left in, sizes counted without them); a left-out bus that has an edge, or a tree too small to rule out a tie with one, settles nothing. _prepare_connectivity passes the base mask and adds it back to each row the tree settles, which is what the search listed for them. Rows that place a branch are still searched. Measured in instructions (callgrind) with the batch driver of benchmarks/cache_profiling, which gains two ScenarioSweep phases -- ss_topo_ac, where one row creates a bus and every other row is plain, and ss_ac, the same rows without actions -- and two-busbar grids (make_grids.py --two-busbars). Per row of ss_topo_ac, before -> after: 1,154,578 -> 1,120,767 on the synthetic 200-bus Illinois grid (-2.9%), 10,291,717 -> 10,071,328 on case1354pegase (-2.1%). The difference is the connectivity pre-pass, for the whole batch 6,979,995 -> 216,579 and 11,966,529 -> 946,436. ss_ac does not move, and every row's voltages are bit-identical before and after. The new BusGraph test checks the tree against the search on random graphs with isolated buses (bus 0 among them). Assisted-by: Claude Code Claude-Session: https://claude.ai/code/session_014PrXNr1rwq4PxLAZ9pL3yu Signed-off-by: Benjamin Donnot <benjamin.donnot@rte-france.com>
The C++ docstring of set_topo_actions and the header comment of _maybe_resolve_topology still described the first version: disconnections only, moves and reconnections refused. Both now say what a row plays (reconnections and moves between busbars included, on one symbolic analysis) and list what is still refused and where. The refusal lists that users read (the binding, the Python wrapper, docs/scenario_sweep.rst and the changelog TODO) gain the half-open branch put back on, refused since the half-open fix; a storage unit in the slack is already covered there by "a slack participant". Assisted-by: Claude Code Claude-Session: https://claude.ai/code/session_014PrXNr1rwq4PxLAZ9pL3yu Signed-off-by: Benjamin Donnot <benjamin.donnot@rte-france.com>
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.
Fixes for the problems a review of #226 found, on top of
fast-env. One commit per fix. Each bug fix comes with a test that failed before it; the commit message says how it failed.ScenarioSweep with topological actions
clear()kept the branch placements (0e52bcb). A batch computed afterclear()without actions replayed the old placements, with solver ids of the old layout. The process aborted on a corrupted heap (munmap_chunk(): invalid pointer).run()raised "call compute first" when the "n" case diverged (c7f951f). The guard now checks that the actions are resolved, not that the base case converged.c62e621).get_violations_n()could report a bus the base grid does not have.53cd131). A row putting such a branch on was solved on a guess: moving its closed end closed the open end too, andset_line_status +1was read as a no-op whileapply_to_gridmodelcloses the open end. Such a row is now refused by name. Disconnecting a half-open branch was already correct and stays allowed.1b8b366). A merge was NOT_SIMULATED withouthandle_disconnected_grid.test_row_splitting_the_gridhas been reworked: its row emptied the bus instead of stranding it, so it now covers both cases.gen_vgroups used the base placement (e6cfcd8). A generator moved off a shared bus was compared with the one left there (row skipped), andset_vmcould seed that bus with the set-point of the generator that left.6d3c3b3). Taking one out left its bus PV. Masking every generator of a bus a storage unit also holds turned that bus PQ. The bus reactive check also leaves a disconnected unit out now.5f1e29b). Disconnecting one re-derives the row's weights. Moving one is refused, as for a generator.4c3ff77). Performance only.BusGraphleaves the known-isolated extra busbars out of its tree. Measured with callgrind through two new ScenarioSweep phases ofbenchmarks/cache_profiling/profile_batch.cpp(two-busbar grids frommake_grids.py --two-busbars). Instructions per row, before → after: 1,154,578 → 1,120,767 (−2.9%) on the synthetic 200-bus Illinois grid, 10,291,717 → 10,071,328 (−2.1%) on case1354pegase. Every row's voltages are bit-identical; table and command in the cache-profiling README.157afa9).30bb8ca).LightEnv
3b05d2f).nb_actions(),get_actions()andinit_actions()now throwstd::logic_errorinstead of dereferencing null pointers.update_rho_time(2d3ae4a). It is now cumulated over the episode and reset with the other timers.Changelog
Two
[FIXED]entries under 1.1.1, for behaviour that shipped in 1.1.0 withset_contingency_gens: a regulating storage unit on a masked generator's bus (6d3c3b3), andmodify_gen_vwith a disconnected generator (e6cfcd8). The half-open refusal is added to the[TODO]list.Testing
ctest, the newBusGraphcase included.unittest, grid2op from source), 620 tests, OK: test_ScenarioSweep_topology, test_batch_redistribute_slack, test_ScenarioSweep, test_ScenarioSweep_gen_contingency, test_ContingencyAnalysis_split, test_ContingencyAnalysis_limit_violations, test_physical_violations, test_lsgrid_violations, test_main_component_slack, test_can_participate_slack, test_cap_slack_at_active_limits, test_binary_serialization, test_storage_pypowsybl, test_voltage_control_batch, test_hvdc_batch, test_fdpf, test_solver_control, test_SecurityAnlysis, test_LightEnv, test_storage_voltage_control, test_storage_distributed_slack, test_InjectionSweep, test_timeseries_sbus.🤖 Generated with Claude Code
https://claude.ai/code/session_014PrXNr1rwq4PxLAZ9pL3yu
Generated by Claude Code