Use PowerIO parser layer and ExaModels backend#48
Conversation
cameronkhanpour
force-pushed
the
mk/backends
branch
from
7ae6103 to
84e78cd
Compare
Benchmark Results (Julia v1)Time benchmarks
Memory benchmarks
|
Rebase completion notePR #48 has now been rebased onto current Implemented scope
Breaking changes
Validation
Manual
|
samtalki
left a comment
samtalki
left a comment
There was a problem hiding this comment.
Nice work, this is a good direction. Dropping the PowerModels runtime dep and getting a ~15x faster parser is a real win, and the Exa parity tests plus the finite difference coverage for transformers, phase shifts, and parallel lines are thorough. A few things before it goes in:
Blocker
docs/Project.tomlhas a hardcodedC:\Users\Bijan\...path in[sources]. That breaks the docs build for everyone except that one machine (it builds locally only because the path exists there). Put it back to{path = ".."}. The SPDX header also got dropped from that file, worth restoring.
Should fix
- The rejected
Dictconstructors keep their entire old body after thethrow.ACNetwork(::Dict)andACPowerFlowState(::Dict)still call_prepare_network_data(deleted) and carry anam = error("unreachable dictionary constructor body")line. It's dead and references symbols that no longer exist. Collapse them down to just thethrow, like you already did forACOPFProblem(::Dict). parse_fileends withresolved isa String ? parse_matpower(resolved; validate) : parse_matpower(resolved; validate)— both branches are identical, so the ternary does nothing. Drop it.
Question
- DC OPF lost the tightened Ipopt tolerances (
tol=1e-10,acceptable_tol=1e-8) in_rebuild_jump_model!, and the thresholds intest_angle_diff_duals.jlgot loosened from 1e-8 to 1e-6 to match. The old comment said the tight tol was needed for accurate dual recovery in sensitivity analysis. Was dropping it intentional?_is_ipopt_optimizeris now unused as a result. If it's deliberate it's fine, just want it to be a choice rather than an accident.
Minor: thermal_lcon/thermal_ucon in _build_examodel get reused for the angle constraints too, so the name is a little misleading; and the angmin_scaled/angmax_scaled arrays in the exa records named tuple look unused (the constraints read b.angmin_scaled per branch).
Otherwise this looks solid.
|
Fixed and pushed PR #48 review follow-up in commit aa5eee5. Changes made:
The loosening on the tolerences was not needed. It was an accidental side effect of dropping the tight Ipopt settings I made during local testings. |
- Gate the AC/Exa `:acceptable` solve warning on `silence()` and state the sensitivity consequence; mirror the DC backend's status messages and add an `:unbounded` branch. - Fix `calc_demand_vector(::ParsedCase)` to index by the sorted `IDMapping` instead of file order, so loads align when bus IDs are unsorted; reuse the caller's `id_map` and add a regression test. - Remove the unused `ACNetwork.i_max` field and the dead `_branch_data_dict`. - Refresh stale PowerModels docstrings (KKT flow equations, `branch_data`, `ACNetwork(Y)`) and document `DCOPFSolution.eta_ref`. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pushed a few quality fixes on top (06a5bde): gated the :acceptable solve warning behind silence() and aligned the AC/Exa solve-status messages with the DC backend; fixed calc_demand_vector to index by the sorted IDMapping (loads misaligned on unsorted bus IDs) plus a regression test; removed the dead i_max field and _branch_data_dict; refreshed the stale PowerModels docstrings. Suite stays green.
Left the parser internals untouched. There are some issues I found. I have a durable solution coming soon and will update this PR ASAP.
samtalki
changed the title
PowerIO = "0.0.1" rides alongside the [sources] pin so the bound is already right when the pin is dropped after registration. README dependency list now names the parser layer. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The release-prep branch merged; main now carries PowerIO.jl 0.1.x with the powerio v0.2.1 binary artifact. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
* Consume PowerIO directly; drop the ParsedCase layer PowerIO.to_powerdata already returns normalized, per-unit, filtered, slack-inferred, cost-rescaled network data, which parser.jl reimplemented as ParsedCase. Build DCNetwork/ACNetwork straight from a PowerIO.Network instead, keeping only PowerDiff's OPF modeling: polynomial cost interpretation, a finite flow-limit fallback when rate_a is 0, default angle-difference bounds, and rejection of storage/HVDC records PowerDiff does not model. - parser.jl: parse_file/parse_matpower return a PowerIO.Network; _network_data builds the network tables; remove ParsedCase/ParsedBus/... and the old normalization helpers. - DCNetwork/ACNetwork/DCOPFProblem/ACOPFProblem/calc_demand_vector take a PowerIO.Network or the network-tables NamedTuple; drop the ParsedCase methods. - IDMapping no longer tracks per-load/shunt ids (loads and shunts are aggregated per bus by to_powerdata). - Read generator costs from PowerIO's raw records: to_powerdata mangles costs declared with ncost>3 (e.g. MATPOWER case14, a quadratic padded to ncost=5). - Finalize PowerIO as a registered dependency: drop the [sources] git pin and set [compat] PowerIO = "0.1". - Migrate the test suite and the IPP experiment off ParsedCase; examples already used the parse_file -> network constructor path. Co-authored-by: Cameron Khanpour <99142483+cameronkhanpour@users.noreply.github.com> Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * Resolve PowerIO from the registry in CI; refresh docs PowerIO is registered and public, so CI no longer needs the private-repo probe. - CI.yml / Documentation.yml / Benchmark.yml: drop the POWERIO_JL_TOKEN env, the PowerIO access probe/skip/configure steps, the availability gates, and JULIA_PKG_USE_CLI_GIT. The test and build (docs) job names are preserved. - docs: drop the removed ParsedCase/Parsed* @docs entries from the API reference and rewrite the PowerIO integration page for the direct to_powerdata path. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * Move get_path (and LazyArtifacts) out of parser.jl get_path resolves PowerDiff's bundled PGLib artifact — a data-library concern, not parsing. Pulling it (and the LazyArtifacts dependency it needs for `artifact"..."`) into src/artifacts.jl leaves parser.jl as just the PowerIO entry points and the network-data adapter. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * Drop PowerIO workarounds; consume to_powerdata directly on 0.1.3 PowerIO 0.1.3 makes to_powerdata a complete data layer: source bus ids on bus_i, an inferred reference (type == 3), and correct polynomial costs (ncost > 3 no longer mangled, so a quadratic padded to ncost=5 keeps its linear term). Bump compat to 0.1.3 and remove the three workarounds _network_data carried for the old gaps: - read gen cost straight from to_powerdata's rows (model_poly/n/c, already per-unit and leading-zero collapsed) instead of re-reading raw costs from PowerIO.generators and rescaling; drop the _cost_tuple helper - drop the biggest-pmax reference promotion; the reference now comes from to_powerdata (type == 3) - drop the try/catch around to_powerdata, which now throws ArgumentError on malformed input itself Kept as consumer-side solver prep (PowerIO leaves these to the caller): the rate_a == 0 finite-limit fallback, the +/-60 deg default angle bounds, rejection of storage/HVDC and PWL/higher-than-quadratic costs, and the dense gen.bus/f_bus/t_bus -> source id mapping. Rename src/parser.jl to src/network_data.jl (it builds network tables, it is not a parser) and move `using PowerIO` to the top of PowerDiff.jl. DCNetwork/ACNetwork field values are unchanged: a before/after field dump over pglib case5/14/30 and a non-basic-id case (ids 1,2,3,4,10) is identical. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * Fold network_data.jl into the DCNetwork construction path The MATPOWER parse wrappers (parse_file/parse_matpower), the _network_data adapter, and its solver-prep helpers (_poly_cost, _fallback_rate_a, _normalize_angle_bounds) now live in types/dc_network.jl rather than a separate file. dc_network.jl is included before ac_network.jl, so ACNetwork and the OPF problem constructors reuse the shared _network_data. This removes the standalone src/network_data.jl, which was just the old parser.jl renamed. Pure relocation: DCNetwork/ACNetwork field values are unchanged (before/after field dump over pglib case5/14/30 and a non-basic-id case is identical), the test suite passes, and docs build with parse_* docstrings resolving from dc_network.jl. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * Address code-review findings on the PowerIO adapter Correctness: - _poly_cost: accept generators with no gencost row (gencost is optional in MATPOWER). PowerIO returns model_poly=false, n=0 for them; treat as cost-free instead of throwing, which had broken even power-flow-only construction. PWL (model_poly=false, n>0) is still rejected, and to_powerdata rejects higher-than-quadratic itself. Consistency: - reject storage from the raw network (PowerIO.storage(net)) like HVDC, so both guards see out-of-service records; to_powerdata's filtered pd.storage dropped them, silently accepting a file that declared disabled storage. Efficiency / clarity: - _poly_cost reads to_powerdata's right-aligned (cq, cl, cc) directly instead of collect/slice/popfirst per generator. - build the branch table with a concrete-eltype comprehension + _branch_row helper instead of an abstract Vector{NamedTuple} + push!. - drop the duplicate per-bus vmax array; _branch_row indexes the buses table. - parse_matpower_struct no longer advertises kwargs... it cannot forward. Docs: - fix stale claims that parse_file returns "PowerDiff's typed representation" (it returns a PowerIO.Network) in README, getting-started, index, advanced. - powerio-integration.md: costs come from to_powerdata, not raw records, and the reference bus comes from to_powerdata, not a largest-generator promotion. No behavior change for valid inputs: DCNetwork/ACNetwork field values are identical (before/after field dump over pglib case5/14/30 and a non-basic-id case), and the test suite passes. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * Re-expose shunts as a data.shunt table Dropping the ParsedCase layer folded shunts into per-bus gs/bs (which the network constructors consume) but removed the separate data.shunt records. Add a `shunt` field back to the _network_data tables: one (; index, shunt_bus, gs, bs) record per bus with a nonzero shunt admittance, derived from the per-bus values to_powerdata already aggregates (no raw re-read). DCNetwork/ACNetwork are unchanged (field dump byte-identical); the inline parser test asserts the restored shunt. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Cameron Khanpour <99142483+cameronkhanpour@users.noreply.github.com> Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR consolidates PowerDiff’s parsing/data ingestion onto PowerIO (removing the prior parser-backend split and PowerModels-dict construction path) and adds an AC OPF backend implemented with ExaModels + NLPModelsIpopt alongside the existing JuMP backend.
Changes:
- Replace PowerModels-dict-based parsing/constructors with
PowerDiff.parse_file/PowerIO.Network+ typed normalization via_network_data. - Introduce ExaModels-based AC OPF backend (
backend=:exa) with parity tests against the JuMP backend. - Update tests/docs/examples to use the new typed/network-table path, and add a PGLib artifact helper (
get_path(:pglib)) plus parser parity tests.
Reviewed changes
Copilot reviewed 49 out of 50 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| test/unified/test_sensitivity_verification.jl | Switch unified sensitivity verification to PowerDiff.parse_file and load_ac_pf_state. |
| test/unified/test_interface.jl | Update unified interface tests for typed network tables and new AC PF state construction. |
| test/test_update_switching.jl | Update switching tests to mutate typed network fields instead of dicts. |
| test/test_sensitivity_coverage.jl | Update sensitivity coverage tests to use new parsing + AC PF state loader. |
| test/test_sensitivity_column.jl | Update column sensitivity tests to use load_ac_pf_state and typed parsing helpers. |
| test/test_parser_parity.jl | New tests asserting PowerIO parser semantics + path/IO parity and typed pi-model checks. |
| test/test_parameter_transforms.jl | Update parameter transform tests to use load_ac_pf_state. |
| test/test_nonbasic.jl | Refactor non-basic tests to operate on PowerDiff network tables and updated IDMapping. |
| test/test_kkt_vjp_jvp.jl | Update KKT VJP/JVP tests to use typed parsing and demand helper. |
| test/test_jvp_vjp.jl | Update JVP/VJP tests to use typed parsing and shared basic-case helper. |
| test/test_dc_opf_verification.jl | Update DC OPF verification perturbations to mutate typed cost coefficients. |
| test/test_apf_integration.jl | Update APF integration to parse via PowerDiff. |
| test/test_ac_topology_sens.jl | Update AC topology FD checks to use pi-model branch current/power helpers and typed PF loader. |
| test/test_ac_opf_sens.jl | Update AC OPF sensitivity tests to typed parsing and typed-network mutations. |
| test/test_ac_opf_exa_backend.jl | New parity/regression tests for ExaModels backend solving + sensitivities. |
| test/test_ac_opf_all_sens.jl | Update all-sensitivities tests to perturb typed-network parameters and cache behavior. |
| test/runtests.jl | Adjust core test harness to typed parsing and include new parser/exa backend tests. |
| test/common.jl | Redefine loaders around PowerDiff typed parsing; add programmatic pd_* helpers + load_ac_pf_state. |
| src/types/show.jl | Add model-status display support for ExaModels models. |
| src/types/id_mapping.jl | Simplify IDMapping to bus/branch/gen only and add NamedTuple constructor. |
| src/types/dc_opf_problem.jl | Update DCOPFProblem constructors to accept PowerIO.Network / network tables; parametrize optimizer type. |
| src/types/dc_network.jl | Implement PowerIO-based MATPOWER parsing + normalization and typed-demand/gen caches on DCNetwork. |
| src/types/ac_opf_problem.jl | Add backend abstraction; build JuMP or ExaModels models from shared typed constants and cached KKT constants. |
| src/types/ac_network.jl | Replace dict-based construction with typed network tables; implement pi-model admittance/current/flow helpers. |
| src/sens/voltage.jl | Remove dict wrapper entrypoint with migration hint. |
| src/sens/vjp_jvp.jl | Require cached KKT constants via _require_kkt_constants. |
| src/sens/topology_ac.jl | Update topology RHS construction for pi-model/tap/shift-aware derivatives. |
| src/sens/interface.jl | Make branch current/flow sensitivity paths work with either ACNetwork or branch_data. |
| src/sens/current.jl | Refactor current/flow sensitivity assembly to use shared state helpers (net or branch_data). |
| src/prob/kkt_dc_opf.jl | Use solution’s stored reference-bus dual in flattening. |
| src/prob/kkt_ac_opf.jl | Remove ref-dict dependency; use cached typed constants for flows/residuals/parameter extraction. |
| src/prob/dc_opf.jl | Capture/store reference-bus dual and gate warning emission on silence(). |
| src/prob/ac_opf_solve.jl | Add Exa backend solve path via NLPModelsIpopt and unify switching rebuild logic. |
| src/PowerDiff.jl | Add PowerIO/ExaModels/NLPModelsIpopt deps, include artifacts helper, export new parsing API. |
| src/artifacts.jl | New get_path(:pglib) artifact resolver. |
| README.md | Update quick start/dependencies for PowerIO parsing and ExaModels backend. |
| Project.toml | Add dependencies/compat entries for PowerIO, ExaModels, NLPModelsIpopt, LazyArtifacts; move PowerModels to test extras. |
| experiments/ipp_market_planning.jl | Update experiment to use typed parsing/networks and typed parameter mutation. |
| examples/interactive_repl.jl | Switch example to artifact-backed PGLib case and typed parsing. |
| examples/apf_integration.jl | Update example to typed parsing and demonstrate typed limit scaling. |
| docs/src/math/ac-opf.md | Update documentation to reflect analytical reduced-space KKT/Jacobian approach. |
| docs/src/index.md | Update docs examples to use PowerDiff parsing (no PowerModels import). |
| docs/src/getting-started.md | Update getting started guide for MATPOWER-only parsing and external AC PF voltages. |
| docs/src/api.md | Document new MATPOWER parser API (parse_file, parse_matpower, get_path). |
| docs/src/advanced.md | Update advanced docs for new construction patterns and Exa backend mention. |
| docs/Project.toml | Remove PowerModels from docs environment deps. |
| docs/powerio-integration.md | New document describing PowerIO contract and PowerDiff normalization responsibilities. |
| benchmark/benchmarks.jl | Update benchmarks to accommodate new parsing/constructors (currently needs a fix per review comments). |
| Artifacts.toml | Add PGLib OPF artifact for bundled case library. |
| .github/workflows/Benchmark.yml | Ensure PowerModels is available for benchmark job environment. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Review — PowerIO parser layer + "drop the ParsedCase layer" (#60)
Overall this is a clean consolidation. _network_data is a tidy single adapter, the HVDC/storage guards correctly read the raw (pre-filter) network so out-of-service records are still rejected (a real subtlety — to_powerdata's filtered output would have silently accepted a file that declared disabled storage/dcline), and the IO/path parity test plus the byte-identical field-dump validation (including the non-basic-id case) give good confidence that the re-keying from to_powerdata's dense indices back to source bus ids is correct. Dropping ~340 lines of native parser for one adapter is the right direction.
A few things before merge.
Blocking
1. benchmark/benchmarks.jl is broken by the ParsedCase removal. (Copilot flagged lines 20 and 38 — both valid.) isdefined(PowerDiff, :ParsedCase) is now always false, so _load_benchmark_case falls into the PM.make_basic_network(PM.parse_file(...)) branch and returns a PowerModels Dict. Line 27 then calls DCOPFProblem(net_data). Unlike the AC path, there is no DCOPFProblem(::Dict) method at all, so this is a raw MethodError, not even the friendly "dictionary constructors were removed" hint. Same for the @benchmarkable parser block (lines 33–39). Replace both branches with PowerDiff.parse_file($case_path); if you still want a PowerModels-parse baseline, add it as a separately-named benchmark instead of gating on the removed symbol.
Consistency
2. Dict-rejection is asymmetric between AC and DC. ACOPFProblem(::Dict), ACNetwork(::Dict), and ACPowerFlowState(::Dict) all throw the migration hint, but DCOPFProblem / DCNetwork have no ::Dict method — a Dict gets a bare MethodError. This is the root cause of how Blocking comment 1 surfaces. Adding matching ::Dict rejection methods on the DC side would make the removed-API story uniform and turn Blocking comment 1 into a clear error if it's ever reintroduced.
Worth confirming
3. _poly_cost assumes g.c is always length 3. src/types/dc_network.jl:303 indexes g.c[1], g.c[2], g.c[3] unconditionally for polynomial costs, relying on to_powerdata right-padding every model-2 gencost to exactly (cq, cl, cc). If a polynomial cost with ncost < 3 (purely linear, or a quadratic whose leading coeff collapsed) ever returns g.c shorter than 3, this is a BoundsError. The inline case and all pglib cases are quadratic (ncost=3), so the tests wouldn't catch it. Either confirm the to_powerdata contract guarantees length-3, or guard/pad before indexing.
Minor
4. AC branch shunt handling centralizes a MATPOWER-only assumption. _branch_row collapses b_fr + b_to into a single br_b; ACNetwork then re-splits it evenly (br_b/2 each) and leaves g_fr/g_to at zero — PowerIO's branch g_fr/g_to are never read. Exact for MATPOWER (symmetric line charging, zero charging conductance) and matches prior field values, but it's now a silent assumption baked into the adapter. Worth a one-line comment, or thread b_fr/b_to/g_fr/g_to through if non-MATPOWER fidelity is ever wanted.
5. Stale doc comments. Same class as Copilot's kkt_dc_opf.jl flatten_variables note (the "eta … set to 0" docstring is stale — it now packs sol.eta_ref). In test/common.jl, the header comment (~line 23) and the load_test_case docstring (~line 69) still say "PowerDiff's typed representation" / "typed parser", but parse_file now returns a PowerIO.Network.
Copilot's three inline findings all look valid; Blocking comment 1 above just expands on the two benchmark ones with the DC-specific MethodError detail.
- benchmark/benchmarks.jl: drop the removed `isdefined(:ParsedCase)` branches and parse with `PowerDiff.parse_file` directly. ParsedCase no longer exists, so the old fallback resolved to `PM.make_basic_network(...)` and `DCOPFProblem(::Dict)` raised a bare MethodError. - Add DCNetwork/DCOPFProblem/DCPowerFlowState(::Dict) rejection methods so the DC side gives the same migration hint as the AC side instead of a MethodError. - _poly_cost: zero-pad model-2 gencosts with fewer than 3 terms instead of indexing g.c[3] (BoundsError guard). - Guard marginal_cost_ub against generator-free networks. - Warn on reference-bus fallback when no type-3 bus is present (AC + DC). - Refresh stale docstrings: flatten_variables eta now packs sol.eta_ref; test/common.jl reflects PowerIO.Network. Document the symmetric branch-shunt and the gen/branch dense-index ID contracts. - Migrate test/mwe_unified.jl off the removed dictionary API. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
@cameronkhanpour all addressed in 9ba0ede:
Full
|
|
your latest commit is failing the benchmark check because |
note: @klamike merely drafted this PR. @cameronkhanpour and @samtalki deserve the credit!
Summary
This branch removes PowerDiff's parser backend split and makes PowerIO the single parser and data layer:
PowerDiff.parse_file(path)callsPowerIO.parse_file(path)PowerDiff.parse_file(io; filetype="m")reads text and callsPowerIO.parse_str(text, "matpower")ParsedCaseorigin/mainThe branch still carries the ExaModels backend work already reviewed in this PR. The parser changes depend on
eigenergy/powerio#69andeigenergy/PowerIO.jl#15.Validation
eigenergy/powerioandeigenergy/PowerIO.jlpublic, or configurePOWERIO_JL_REPO_TOKENwith read access so CI can instantiate PowerIO.jl.