Fix derivatives

[BDonnot]

Fix some issues by testing the batch algorithm against finite differences

[BDonnot]

Some changes

[BDonnot]

remove RTE please

[BDonnot]

xml becomes an hard dependency then ? From memory it's rather heavy :-/ Is there any solution outside of it ? Or maybe it's already a dependency of pypowsybl (it would surprise me)

Maybe lazy incluse it if needed only. With a try / catch failing back to a reasonable default behavior if not installed.

[BDonnot]

Not a new dependency: xml.etree.ElementTree is in the Python standard library, so it is already there with the interpreter, and neither pypowsybl nor anything else has to ship it. Its C accelerator (_elementtree) is built into CPython.

That said, the concern about paying for it on every import is fair, so it is now imported inside _apc_from_xiidm — the only function that needs it, and only on the fallback path (pypowsybl <= 1.16.1, source="auto"). The import is wrapped in a try / except ImportError that warns and returns {}, which makes every battery fall back to OpenLoadFlow's defaults, so a Python built without _elementtree still loads the grid.

Done in c16eb3f (branch claude/pr-207-comments-defow7).

---

Generated by Claude Code

[BDonnot]

I put it here because I don't know where to have it otherwise:

• in the previous merge, there was also capability to detect elements of the slack outside their pmin / pmax. Is this physical detection present for battery too ?
• another question, still about the previous (already merged) feature, are the pmin / pmax of generator read when the grid is init from Matpower/ powermodels and pypowsybl ?

[BDonnot]

Two questions, two different answers.

1. Is the pmin / pmax detection present for batteries too? No, and it cannot be yet.

GenPCheck (the active-power third of compute_physical_violations) only walks the generators, and the reason is one level down: StorageContainer carries no active power limits at all — there is no min_p_mw / max_p_mw on a storage unit, no set_storage_p_limits, nothing in StateRes. So a battery the distributed slack pushed past what it can deliver goes unreported, even though this PR does let it take a share of the slack.

Closing that gap is a real piece of work (container state + BINARY_FORMAT_VERSION bump + bindings + the converters + GenPCheck), so I have not widened this PR with it — it is written up as a TODO in the changelog instead, with what it takes. Say the word if you would rather have it here.

The reactive side is fine: a voltage-regulating storage unit's [min_q, max_q] does count towards its bus' capability (BusQCheck), tested both in test_physical_violations.py::TestStorageCapabilityFromPython and in test_batch_physical_violations.cpp.

2. Are the generators' pmin / pmax read by the other converters? pypowsybl yes, MATPOWER / PowerModels no — now fixed.

• pandapower: yes, _aux_add_gen.py reads min_p_mw / max_p_mw.

• pypowsybl: yes, _aux_add_generators.py reads min_p / max_p (iidm always carries them).

• PowerModels / MATPOWER: no. from_powermodels/_aux_add_gen.py read qmin / qmax and stopped there, so a grid loaded from a .m file could not report an active-power violation at all — even though _mpc_to_powermodels.py was already carrying PMIN / PMAX through into the dict, unused.

  Fixed in c16eb3f: the PowerModels converter now calls set_gen_p_limits, which covers MATPOWER since it goes through the same dict. A missing or non-finite limit becomes NaN, i.e. "no limit for that one", same convention as the other two. Tested in test_init_from_matpower.py::test_generator_active_power_limits_are_read.

---

Generated by Claude Code

[BDonnot]

Avoid redeclaring this type of constants. The day it changes it will be error prone (and hard to debug). Have them all in an util file that is imported when such constants are needed.

[BDonnot]

Agreed — and worse than a duplicate, _aux_battery_apc.py was importing two of them back from _olf_bake.py, which is what forced the lazy-import dance between the two modules.

They now all live in one new module, lightsim2grid/network/from_pypowsybl/_olf_const.py, in two groups: OpenLoadFlow's own constants mirrored here (_ZERO_P_TOL, _MIN_REACTIVE_RANGE_MVAR, _MIN_PLAUSIBLE_TARGET_V_PU / _MAX_PLAUSIBLE_TARGET_V_PU, _MIN_NOMINAL_V_FOR_TARGET_V_CHECK_KV, _MAX_PLAUSIBLE_ACTIVE_POWER_MW, _OLF_DEFAULT_DROOP) and the tolerances used to read a reference solve back (_Q_LIMIT_TOL_ABS / _Q_LIMIT_TOL_REL, _TARGET_V_HELD_TOL_PU, _TARGET_Q_TOL_MVAR). _olf_bake.py has no module-level constant left at all, _aux_battery_apc.py and _aux_add_slack.py import from _olf_const directly, and the circular import is gone with them.

Done in c16eb3f (branch claude/pr-207-comments-defow7).

---

Generated by Claude Code

[BDonnot]

Remove rte and use a more generic term "real grid snapshots" for example

[BDonnot]

Mention of rte

And 42% should not happen here

[BDonnot]

Putting it at the top of the changes here, but does not really concern it:

Add in the Todo, on the Changelog, the fact that :

1. this file should be split into comprehensible "sub files" to facilitate the debugging
2. there should be, opt-in options, off by default, the possibility to get the changes made to the grid. Like "gen xxx removed from slack", "batt yyy assigned PQ" etc in a json format for example

[BDonnot]

Both added at the top of the [TODO] section of CHANGELOG.rst (c16eb3f):

1. Split _olf_bake.py. Along what each rule bakes — taps and sections, generator voltage control, reactive-limit switches, SVCs, active power and slack participation — so a failing bake points at one readable module rather than at a thousand-line file.
2. An opt-in report of what the bake changed, off by default: "gen xxx removed from the slack participants", "batt yyy frozen to PQ", "tap zzz fixed at position n", in json, each with the rule that decided it. Today the bake rewrites the grid silently and a caller cannot tell which decision produced the state it is handed.

Two more went in while going through the review, from the other threads: separating slack participation from the reference slack (here), and active-power limits for storage units (here).

---

Generated by Claude Code

[BDonnot]

Rte...

[BDonnot]

Are storage units properly added to the physical violation qmin / qmax check ?

[BDonnot]

Yes — a voltage-regulating storage unit's reactive range does count towards its bus' capability.

The STORAGE tag not being a controller of the voltage-control plan is what made this look doubtful, and the two are independent: BusQCheck::build_bus_q_plan builds each bus' [q_min, q_max] from everything that holds that bus, whatever path it holds it through, and it walks the storage units explicitly (is_local_voltage_controller, get_min_q / get_max_q summed into the bus like a local generator's). Tested in test_physical_violations.py::TestStorageCapabilityFromPython::test_limits_of_a_regulating_storage_unit and in test_batch_physical_violations.cpp ("a voltage-regulating storage unit's reactive capability counts towards its bus'").

Since the question came up right here, the comment on Kind now says it rather than leaving it to be re-derived (c16eb3f).

The active side is the one that is genuinely missing — see my answer on the compare_lsgrid.py thread: a storage unit has no min_p_mw / max_p_mw at all, so GenPCheck has nothing to compare against. TODO added.

---

Generated by Claude Code

[BDonnot]

I put this comment here, why not...

Add a Todo in the changelog for the NR behavior when multiple slacks are present.
We should now make the difference between "participate in the distributed slack" which is pure active things and has nothing to do with voltages and "is reference slack" which is a much stronger say : theta = known, VM = known, p and q unknown.
Today an element participating to the distributed slack is necessarily PV which does not make any sense (a load could participate in the distributed slack...)

Let's not adress this yet, but have a todo around this.

[BDonnot]

Agreed, and not addressed — TODO added, in two places (c16eb3f):

• CHANGELOG.rst, top of [TODO]: "participates in the DISTRIBUTED SLACK" is a purely active statement — this element takes a share of the power imbalance, by a participation factor — and says nothing about voltage, unlike "is the REFERENCE slack" (theta known, |V| known, P and Q unknown). Today a participant is necessarily PV, which does not follow: a load could take a share without controlling any voltage. Separating the two roles is what would allow it.
• a shorter note in the class docstring of SlackParticipation itself, pointing at the changelog — that is where someone adding a participant will be looking.

---

Generated by Claude Code

[BDonnot]

Review worked through — on branch claude/pr-207-comments-defow7

All 20 threads are addressed in c16eb3f, pushed to claude/pr-207-comments-defow7 (branched off this PR's head, eccddec), so it merges into fix_derivatives cleanly. I did not push to your branch.

	added	removed
C++ (src/core)	+11	-1
Python (package)	+150	-122
Tests	+10	-3
Docs + changelog	+48	-1
total	+296	-162

RTE names and measured numbers — 11 threads

Every RTE introduced by this PR is gone, replaced by "a real grid snapshot" / "real grid snapshots", and every number carried over from an investigation with it. What each rule does is said instead:

where	was	now
_aux_battery_apc.py:25	"a real RTE snapshot ... shares x12.4, x1, x0.144"	which units the rule keeps, and that the droop = 0 ones take no part
_aux_add_slack.py:58	"7k-bus RTE snapshot ... to 1e-13"	"real grid snapshots ... to solver precision"
_olf_bake.py:71	"30 generators ... and ~100 they left regulating"	what the catch-all decides, in both directions
_olf_bake.py:214-215	"two units at 1.22366 and 1.22242 reported 1.22304 and 1.22242"	two units of a bus reported a hair from what they injected, the bus no longer balancing
_olf_bake.py:229-232	"2.9e-4 and 6.2e-4 MVAr ... 0.18 MVAr inside it ... 0.11 MVAr beyond"	the bus balance being off by the discrepancy is what identifies a misreport
_olf_bake.py:418	"~1e-16 pu ... at least ~1e-7 pu away"	machine precision vs. orders of magnitude further; the threshold sits between them
_olf_bake.py:734-737	"RTE snapshots ... 7.6e-4 pu on the 6 kV side"	the case it covers, and that the low-voltage side is visibly off without it
_olf_bake.py:825-826	"85.8 MW on a 217-947 MW curve ... 18.820306 / -13.686818 MVAr"	the extrapolated limit sits far enough from the clamped one to bake the wrong value
_olf_bake.py:909	"2e-3 pu on an RTE snapshot"	removed
_olf_bake.py:1117-1118	"the 41 units at their max_p carried 42 %"	the capped units carry enough of the raw key that ignoring them skews the distribution
test_olf_bake.py:499	"on RTE snapshots"	"on real grid snapshots"

The numbers that stay are the ones a reader can act on: the tolerances the code uses, the constants mirrored from OpenLoadFlow, the values the test fixtures set.

The rule itself is now written down in CLAUDE.md (Comments and docstrings: no RTE, no measured numbers), so it does not have to be re-discovered next time.

The rest

• Constants — all of them, OLF-mirrored and reading-back tolerances alike, moved into one module from_pypowsybl/_olf_const.py. _olf_bake.py has no module-level constant left (that also answers "constants at the top of the file"), and the circular import between _olf_bake and _aux_battery_apc goes with them.
• xml — stdlib, not a new dependency, but now imported inside _apc_from_xiidm only, behind a try / except ImportError falling back to OpenLoadFlow's defaults.
• Merging the two voltage-control functions — the decision merged (_resolve_regulated_bus, _target_v_held), the read/write around it did not, because a battery's regulation lives in an extension frame. Reasons in the docstring and on the thread.
• Storage units and the physical checks — reactive: already covered (BusQCheck counts a regulating storage unit's range in its bus'). Active: not covered, and cannot be — a storage unit has no min_p_mw / max_p_mw at all. TODO added rather than widening this PR.
• Generator pmin / pmax by converter — pandapower yes, pypowsybl yes, PowerModels / MATPOWER no. Fixed: the PowerModels converter now calls set_gen_p_limits, which covers MATPOWER too, with a test.
• TODOs — four in CHANGELOG.rst: separating distributed-slack participation from the reference slack (+ a note in SlackParticipation.hpp), splitting _olf_bake.py, the opt-in json report of what the bake changed, and storage active-power limits.

Tests

Built from source and run locally: test_olf_bake (27), test_storage_voltage_control (8), test_storage_distributed_slack (12), test_init_from_matpower (27), test_voltage_control_pypowsybl (20), test_physical_violations (26) — all pass, except 4 in test_physical_violations that error on import grid2op, which is not installed in this container and is unrelated to the change.

---

Generated by Claude Code

[BDonnot]

Ok