Repository navigation
Conversation
|
@BDonnot Thanks for this — connecting lightsim2grid as a third PF engine, and doing it by mirroring the powsybl layout, makes for a clean and easy-to-follow addition. Against our CONTRIBUTING checklist this is in very good shape already: new dep in What's needed
Nothing above is a merge blocker in my view — I'll leave the merge/approval call to a maintainer. — 🤖 _automated pre-review; a maintainer will follow up_ |
|
@romeokienzler Could you please guide @BDonnot on how to make sure the pytest tests for the new Thank you both :) |
|
@albanpuech @BDonnot Happy to. The tests SKIP because CI never installs the solver: the main pip install -e ".[test,dynamic,powsybl]"
...
pytest tests/dynamic tests/powsybl -vThe lightsim2grid extra already exists in
(A dedicated One thing to confirm once it runs: pip will pull a released @BDonnot — this is a CI-only change ( |
|
Hello @romeokienzler
The lightsim2grid >= 1.1.0 (and even 1.0.0 to be honest) is enough to make this powerflow working. What is missing there (I just merged the feature in the development branch of lightsim2grid). This is why I put a try / except, maybe a bit broad (Exception and not AttributeError) I will fix it and replace it with : try:
_update_in_place(ls_net, net, mapping, old)
except AttributeError:
# half-updated: nothing about the LSGrid can be trusted anymore
return to_lightsim2grid(net)
I did not dare changing the CI but that is definitely something I can do. I'll match what is done for pypowsybl and dynawo. Thanks Benjamin |
|
@BDonnot Both plans sound right to me.
One thing to eyeball on the first green run: since CI will pull a released lightsim2grid without the in-place methods, it exercises the rebuild path — so confirm Also please rebase — the branch is |
lightsim2grid replaces the PF solver in PF mode, next to powermodel and powsybl:
settings:
pf_solver: lightsim2grid
The network is read with the native reader, then converted to a lightsim2grid
LSGrid straight from the Network arrays (init_from_matpower: no pandapower,
no pypowsybl). OPF is still solved by PowerModels.
New package gridfm_datakit/lightsim2grid, laid out like gridfm_datakit/powsybl:
- api: availability check (is_lightsim2grid_available, check_...)
- mapping: index maps between the Network and the LSGrid (buses and generators
keep the row order, branches are split in powerlines and transformers)
- convert: to_lightsim2grid, update_lightsim2grid, initial_voltage
- preprocess: run_ls_pf / get_pf_res, which format the AC or DC result like the
PowerModels one so pf_post_processing consumes it unchanged
update_lightsim2grid pushes only what changed since the last synchronisation
(branch parameters and statuses, generator statuses and set points, loads) into
the same LSGrid. It rebuilds it when something it cannot update changed
(topology, tap ratio, phase shift, shunts, bus types, which buses have a load,
the slack generators). The worker keeps the LSGrid in meta, so it is reused
across the perturbations and the scenarios it processes. The lightsim2grid
setters ignore changes of at most 1e-7, so such a change is applied through a
detour to keep the LSGrid exactly equal to the Network (otherwise the data
would depend on the previous scenarios processed by the worker).
In-place parameter updates need the update_powerlines_parameters /
update_trafos_parameters methods of lightsim2grid. The new optional dependency
`lightsim2grid` (pyproject.toml) is pinned to >=1.1.0: it has to be moved to a
release that contains them.
Checked on case14 (the AC results match PowerModels' fast PF to 1e-6 on every
column; the DC ones differ by design: lightsim2grid uses the MATPOWER DC model,
1/(x.tap), PowerModels x/(r^2+x^2)) and timed on case118 and case2000.
Assisted-by: Claude Code (claude-sonnet-5)
Signed-off-by: DONNOT Benjamin <benjamin.donnot@rte-france.com>
…olver - tests/lightsim2grid/test_generate.py had the same module name as tests/test_generate.py (tests/lightsim2grid is not a package: it cannot be one, it would shadow the lightsim2grid package), which made `pytest tests/` fail at collection. Renamed to test_lightsim2grid_solver.py. - Apply the pre-commit hooks (ruff-format, add-trailing-comma). - Google-style Args / Returns / Raises sections for the functions of gridfm_datakit.lightsim2grid, and a docstring for _update_in_place. Drop the unused `mapping` argument of _snapshot. Assisted-by: Claude Code (claude-sonnet-5) Signed-off-by: DONNOT Benjamin <benjamin.donnot@rte-france.com>
- New manual page "Power flow solver": the three engines, how the lightsim2grid model is built and kept up to date, and how its results differ from PowerModels' (AC agrees, the DC model is not the same). - New components page for gridfm_datakit.lightsim2grid, listed in mkdocs.yml. - getting_started.md: pf_solver in the settings and in the PF mode notes. - installation.md: the `lightsim2grid` extra. - pf_solver (default "powermodel", options powermodel / powsybl / lightsim2grid) in scripts/config/default.yaml and tests/config/*.yaml. `mkdocs build` and `pre-commit run --all-files` pass. Assisted-by: Claude Code (claude-sonnet-5) Signed-off-by: DONNOT Benjamin <benjamin.donnot@rte-france.com>
The update_powerlines_parameters / update_trafos_parameters methods that gridfm_datakit.lightsim2grid uses to update the model in place are on the dev_gfm_datakit branch of lightsim2grid, not in a release yet. Until one has them: - installation.md: install lightsim2grid from that branch, then the extra, and what happens without the methods (same results, model rebuilt every time); - power_flow_solver.md: the version note points to it; - pyproject.toml: a TODO on the lightsim2grid extra to move its lower bound to the first release that has them. Assisted-by: Claude Code (claude-sonnet-5) Signed-off-by: DONNOT Benjamin <benjamin.donnot@rte-france.com>
b2ed8ce to
ce75614
Compare
…m2grid tests in CI update_lightsim2grid caught a bare Exception before falling back to rebuilding the LSGrid; narrow it to AttributeError, the only failure expected on a released lightsim2grid that lacks update_powerlines_parameters / update_trafos_parameters, so a genuine bug in _update_in_place surfaces instead of being silently swallowed. dynamic-pytests already installs the heavy solver extras and runs the powsybl/dynawo tests; fold lightsim2grid into that job the same way so tests/lightsim2grid stops being skipped in CI. Assisted-by: Claude Code (claude-sonnet-5) Signed-off-by: DONNOT Benjamin <benjamin.donnot@rte-france.com>
ce75614 to
39ee1da
Compare
|
@BDonnot Both changes landed exactly as discussed — nice.
That covers everything I'd raised. One thing to eyeball on the first green DCO is green and the branch is mergeable; the |
|
What I did:
(plus an issue with DCO...) |
|
@BDonnot Thanks — your checklist matches what I confirmed on the last pass (rebase, On the DCO hiccup: it's green now — all five commits carry The only thing still worth a glance is the one I flagged before: on the first CI Otherwise this looks ready for a maintainer's look — approval/merge is their call. — 🤖 _automated pre-review; a maintainer will follow up_ |
|
Oh sorry I missed your comment on that. Indeed, it is exercising only the "rebuild" pass as of now, just like someone who would use the datakit right now. So this test is not exercising what it claims to do (yet) this is a matter of days, 2-3 weeks at most. |
|
@BDonnot No worries — and your read is exactly right: right now The one thing worth doing so the in-place coverage doesn't quietly fall through the cracks in a few weeks: leave a small marker for future-you — e.g. a Everything I raised earlier is resolved — this looks ready for a maintainer's look; approval/merge is their call. — 🤖 _automated pre-review; a maintainer will follow up_ |
|
(It is Alban talking to you, not claude code ;) ) The integration looks great so far. I did this small test. I think you will like it Ran What matched
The last row of the table is really good news! Speed
lightsim2grid is faster at power flow. The full run is not, because both paths still do OPF with PowerModels, and that step dominates (difference is noise) DC PF residuals are not the same because the solvers use different DC models: lightsim2grid follows MATPOWER ( |
|
I made claude happy (trying to at least) with the fix test, I will push ASAP :-) |
|
cool! Lmk what you think of the convergence/ speedup/ validation results above :) |
romeokienzler asked for a marker so the in-place-update coverage of tests/lightsim2grid isn't quietly lost: CI installs a released lightsim2grid, which has no update_powerlines_parameters / update_trafos_parameters yet, so test_in_place_update_matches_a_rebuild was actually asserting the wrong thing there (n_rebuilt == 1, when every branch-parameter change in fact falls back to a rebuild -- verified locally by removing those methods from the LSGrid class). Only assert the exact rebuild count when the LSGrid supports the in-place path, and cross-reference the matching TODO between pyproject.toml and the test so bumping the lightsim2grid lower bound to the release that has these methods flips CI onto asserting n_rebuilt == 1 again automatically. Assisted-by: Claude Code (claude-sonnet-5) Signed-off-by: DONNOT Benjamin <benjamin.donnot@rte-france.com>
|
@BDonnot That's exactly the fix — thanks for chasing it down.
CI just re-triggered on this push and is still pending; worth a glance once |
|
The speed-ups are (much) lower than what I observed outside of the datakit (but close to what I observed on my machine with the datakit). But I think it's because the datakit does not optimize the same way. One grid at a time is much slower than computing a batch of grids. But still 4x faster is pretty nice already :-) |
|
I'll fix the failing tests tomorrow. Probably something I missed with the lightsim2grid version on pypi... Sorry about that |
|
@BDonnot No need to apologize — and this is actually the system working as intended: the red check is One pointer to save you time tomorrow: your last commit ( No urgency — tomorrow is totally fine. Everything else stands where it was, and merge/approval remains a maintainer's call. — 🤖 _automated pre-review; a maintainer will follow up_ |
|
@BDonnot Thanks — the new commit (
One new thing this push introduced, though:
Once DCO is back to green, this looks ready for a maintainer's look — the CI fix itself is sound. Approval/merge remains a maintainer's call. — 🤖 _automated pre-review; a maintainer will follow up_ |
… lightsim2grid _update_in_place looked up update_powerlines_parameters / update_trafos_parameters unconditionally, so on lightsim2grid 1.1.0 (which has neither) every update raised AttributeError and rebuilt the LSGrid, even for load or generator changes only. This failed test_in_place_update_applies_changes_below_lightsim2grid_tolerance in CI. Look the method up only when branch parameters actually changed. Assisted-by: Claude Code Signed-off-by: DONNOT Benjamin <benjamin.donnot@rte-france.com>
7568d14 to
45f8775
Compare
|
@BDonnot Thanks — DCO is green again. The latest commit ( That was the only thing I had outstanding. Everything raised earlier (rebase, Looks ready for a maintainer's look; approval/merge remains their call. — 🤖 _automated pre-review; a maintainer will follow up_ |
|
@BDonnot Just to let you know I will need some time to go thru all this... will do my best to review before our meeting with Nicolas and François |
|
No problem, honestly it's almost a copy paste from the pypowsybl one : pypowsybl being more complex I mainly "simplified" it and change the calls to use lightsim2grid. No hurry on my side however Best Benjamin |
|
@BDonnot Just one new thing since the last pass — no urgency, and nothing in the code review changed.
Everything substantive from before stands as addressed; this is purely the branch going stale against |
Resolve the conflicts in docs/installation.md and mkdocs.yml by keeping both the lightsim2grid and the PowSyBl sections and nav entries. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MmzXydFH577XovfunJPjhi
|
@BDonnot Thanks — the merge-from-main (
Everything substantive from the earlier passes stays addressed; this was purely the branch going stale against |
|
LGTM! Nothing to flag from a code review perspective. @albanpuech, I see you started running some benchmarks. Did anything come up that would block approval? |
With some discussion from example with @albanpuech I tried to "connect" lightsim2grid with the datakit.
To a relatively good surprise, this was done rather simply (I took the powsybl integration as an example).
I tested the data generation process on the cases 14, 118 and 2000 with no actions and some random actions.
I also made a branch in lightsim2grid: https://github.com/Grid2op/lightsim2grid/tree/dev_gfm_datakit to further speed up the computation. Wie the released lightsim2grid version on pypi you have to build the grid from scracth each time when the physical parameters (r,x, g, or b) are modified.
The development here does not depend on it, but if lightsim2grid is installed from source from this branch (instruction in the docs) this will make lightsim2grid even faster.
This developements have been assisted by Claude (Sonnet 5), all generated code has been reviewed and validated by myself.
I also noticed that there were discrepencies between DC pf between lightsim2grid and powermodels. For DC modelling, lightsim2grid strictly follow the matpower equations (heavily tested). I am not quite sure what is the convention used by powermodels.
Let me know if something is missing
Best
Benjamin