Conversation
initialize_LCCParameters! only seeded lcc_rectifier_tap, lcc_inverter_tap, lcc_rectifier_delay_angle, and lcc_inverter_extinction_angle into column 1, leaving time_steps>=2 at their zero allocation default. Any multiperiod LCC solve (time_steps>1) hit a singular Jacobian at step 2 onward regardless of discrete control. Broadcast the setpoint into every column instead, matching how lcc_p_set and lcc_i_dc are already seeded.
…comment Runs the repo formatter over the branch's changed files (destructuring signatures in control_continuation.jl and two test assertion blocks in test_lcc_discrete_control.jl reflow to the formatter's line width). Also extends the rollback contract comment on _capture_state!/_restore_state! to list the LCC solver state (taps, thyristor angles, DC current) alongside bus and VSC state, and notes that derived caches are re-derived rather than snapshotted.
The paragraph described the checkpoint as the full PowerFlowData state rolling back discrete device moves; in fact the checkpoint covers only V/theta, bus_type, the injection columns, and the VSC/LCC columns, while device moves are undone separately by apply_parameter! at the call sites. Also corrects "snapped" to "restored" (snap is reserved for discrete-grid snapping elsewhere in this doc), notes phi among the re-derived LCC caches, and clarifies that the orientation probe captures/restores the checkpoint on every control-enabled solve, not only on a failed attempt.
… coverage, vacuity guard The checkpoint restore test hardcoded the five checkpointed columns by name, so a field newly classified as CHECKPOINTED could be added without extending the checkpoint and this test would never notice. It now iterates LCC_CHECKPOINTED_FIELDS via getfield, assigns distinct garbage per column, and asserts the snapshot tuple length against the classified column count. Adds a non-polar coverage test (ACRectangularPowerFlow + LCC + control_discrete_devices), since the FD plant-sign probe path this exercises had no direct test. Adds a vacuity guard to the multiperiod equivalence test: asserts steps 1 and 3 of the solution differ by more than the "probe restores do not leak across steps" isolation test's comparison tolerance, so that isolation test cannot pass simply because there was nothing to tell apart.
| lcc_i_dc .= _lcc_i_dc_from_p_set.(lcc_dc_line_resistance, lcc_p_set) | ||
| lcc_rectifier_delay_angle[:, 1] .= PSY.get_rectifier_delay_angle.(lccs) | ||
| lcc_inverter_extinction_angle[:, 1] .= PSY.get_inverter_extinction_angle.(lccs) | ||
| lcc_rectifier_delay_angle .= PSY.get_rectifier_delay_angle.(lccs) |
There was a problem hiding this comment.
Is this actually a pre-existing bug fix or was there a reason this was only applied to the first time step that I am missing?
There was a problem hiding this comment.
Good catch, yep that's a bug. When we added support for multi-period AC power flow (from within PF) the buses, voltages, etc. got updated to broadcast the system value to all time steps, but evidently we missed the LCCs.
luke-kiernan
left a comment
luke-kiernan
left a comment
There was a problem hiding this comment.
The implementation itself looks fine. However, I'm struggling to make anything of the tests, to tell if they're quality or just token coverage. A good part of this is probably AI's awful commenting practices: it's happy to write verbose paragraph but neglects to provide basic signposts.
| lcc_i_dc .= _lcc_i_dc_from_p_set.(lcc_dc_line_resistance, lcc_p_set) | ||
| lcc_rectifier_delay_angle[:, 1] .= PSY.get_rectifier_delay_angle.(lccs) | ||
| lcc_inverter_extinction_angle[:, 1] .= PSY.get_inverter_extinction_angle.(lccs) | ||
| lcc_rectifier_delay_angle .= PSY.get_rectifier_delay_angle.(lccs) |
There was a problem hiding this comment.
Good catch, yep that's a bug. When we added support for multi-period AC power flow (from within PF) the buses, voltages, etc. got updated to broadcast the system value to all time steps, but evidently we missed the LCCs.
luke-kiernan
left a comment
luke-kiernan
left a comment
There was a problem hiding this comment.
A few small things
| # Twin-parity tolerance, sized between the two quantities it has to separate: the measured | ||
| # twin error is ~1.5e-8 in bus angle, while the per-step spread the multiperiod tests rely on | ||
| # is ~2.4e-5 in bus angle and ~1e-6 in voltage magnitude. | ||
| const LCC_PARITY_ATOL = 1e-7 |
There was a problem hiding this comment.
I'm still not a huge fact of the fact that the voltage magnitude spread is only 1 order of magnitude above the tolerance. I had Claude checks the numbers against pencil-and-paper estimates and it found no issues, though, so it's not a big concern.
Aside: odd detail from Claude's math. Not sure if it's relevant, though.
Note R/X ≈ 5–23 — this network is resistance‑dominated, so the usual P↔θ / Q↔V decoupling is inverted: ΔQ moves angle, and only weakly moves magnitude.
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
2 participants
This closes #422