Act on power flow convergence instead of always reporting solved - #309
luke-kiernan wants to merge 3 commits into
Conversation
`IOM.evaluate!` discarded what `PFS.solve_power_flow!` reported and set `is_solved = true` unconditionally, so a non-converged time step reached consumers as NaN-poisoned state with no error: the platform's named silent-failure pattern. Read `PFS.get_converged(pf_data)` and set `is_solved = all(converged)`, logging the failed time steps. `get_converged` rather than the return value of `solve_power_flow!`, since the DC methods return `nothing` while both AC and DC populate `converged`. The single-period branch (PSSEExporter) only writes data out and has no convergence to report, so it keeps setting `true`. Fixes #305 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
🟢 Approval recommended
The convergence handling and regression coverage address the stated behavior change with no blocking issues identified.
Pull request overview
Updates power-flow evaluation status to reflect actual multi-period convergence, preventing false successful solves.
Changes:
- Marks evaluations solved only when all time steps converge.
- Logs failed time steps while preserving single-period exporter behavior.
- Adds regression tests for full and partial convergence.
File summaries
| File | Summary |
|---|---|
test/test_power_flow_in_the_loop.jl |
Tests successful and partial convergence behavior. |
ext/PowerFlowsExt/pf_solve_and_aux.jl |
Applies convergence status to power-flow evaluations. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 0
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
acostarelli
left a comment
acostarelli
left a comment
There was a problem hiding this comment.
Why say solve! failed when PF didn't converge but the PCM did? I feel like those should be separate: solve can still succeed but pf.is_solved may be false.
PFs can fail for a multitude of reasons on a feasible decision model |
|
For sure, but why are we saying that the whole |
Oh I missed that detail when opening this PR via Claude code. Yeah that's a pretty big change. This is really pending a design choice: see comment here and the alternative I laid out here...but based on the issue Jose opened, I was inferring that failure-to-solve should be treated as an error. I guess the issue that |
Because the PFs in PSI are aux variables, we don't want to stop a simulation that uses a DC assumption to fail because it can't recover all the steps of an AC PF for example. |
|
but based on the issue Jose opened, I was inferring that failure-to-solve should be treated as an error -> yes but not a simulation fail. We talked about using NaN or something else in the aux vars |
`latest_solved_power_flow_evaluation_data` searched for an evaluator with `findlast` over a `Dict`, which returns an arbitrary one once more than one is registered: keys owned by the other evaluator were then silently never written (issue #308). IOM now passes the evaluator down from its loop (InfrastructureOptimizationModels.jl#168), so the read-back filters on ownership and writes, with no search. A non-converged power flow no longer fails the run either: PowerFlows NaNs out the time steps that failed and `evaluate!` logs them, so the aux vars carry the failure while the optimization's own status stands. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Performance Results
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Fixes #305 and #308. Depends on Sienna-Platform/InfrastructureOptimizationModels.jl#168.
#305 — convergence was thrown away.
IOM.evaluate!ignored whatPFS.solve_power_flow!reported and setis_solved = trueunconditionally. It now readsPFS.get_converged(pf_data), setsis_solved = all(converged), and@errors with the failed time steps. Readingget_convergedrather than the return value because the DCsolve_power_flow!methods returnnothing; both AC and DC populateconverged. The single-period branch (PSSEExporter) has no convergence to report and still setstrue.A non-converged power flow does not fail the run. It is an error in the log and NaN in the aux variables: PowerFlows already blanks the diverged time steps (
OVERWRITE_NON_CONVERGED), and the optimization itself solved fine, sosolve!still returnsSUCCESSFULLY_FINALIZED. This is also strictly more than the old code gave you, where one bad time step threw and erased all the aux var data. Per-step tolerance is the remaining open part of #273.#308 — the read-back picked an arbitrary evaluator.
latest_solved_power_flow_evaluation_datafound one withfindlastover aDict, whosevaluesorder is arbitrary with more than one entry. Keys owned by the evaluator it didn't return hit the ownership filter's|| returnand were silently never written. IOM'scalculate_aux_variables!now passes down the evaluator it is already holding, so that helper is deleted: the ownership filter moves onto a 4-arg method and the two concrete writers become_write_aux_variable_value!.Tests. The non-convergence case builds a CopperPlate case whose LP is unaffected by the network, with two lines weakened enough that only the peak hours diverge — so
all(converged)is doing real work, not just distinguishing all-pass from all-fail — and asserts NaN aux vars at exactly those hours. Tuning is noted in a comment; it is calibrated against the current NR solver. Two multi-evaluator testsets cover #308: AC + PSS/E export (the realistic pairing, and the worst case forfindlast— the exporter owns no aux vars, so picking it skipped everything), and AC + DC, which checks order-independently that the DC evaluator leaves an AC-only key alone while the AC evaluator writes it.🤖 Generated with Claude Code