Update MT-HVDC quadratic converter model#103
Open
acostarelli wants to merge 9 commits into
Open
Conversation
acostarelli
commented
Apr 29, 2026
rodrigomha
reviewed
Apr 29, 2026
| @test solve!(model) == IOM.RunStatus.SUCCESSFULLY_FINALIZED | ||
| end | ||
|
|
||
| @testset "HVDC System with Losses Network (QuadraticLossConverter NLP)" begin |
Contributor
There was a problem hiding this comment.
We should add a test that have both together and check that they are in the same order of magnitude for current and dc power probably.
You can run the same system and run without linear loss for both.
Contributor
There was a problem hiding this comment.
Pull request overview
Updates the MT-HVDC quadratic-loss converter modeling to support both a MIP-friendly Bin2/PWL approximation and an exact (nonlinear) formulation, and expands tests to validate both paths.
Changes:
- Introduces
AbstractQuadraticLossConverterwith two concrete formulations:Bin2QuadraticLossConverter(PWL/SOS2+Bin2) andQuadraticLossConverter(exact bilinear/quadratic, NLP-capable). - Refactors MT-HVDC converter construction/constraints to use shared bilinear/quadratic expression builders instead of bespoke “squared”/interpolation variables.
- Updates HVDC tests and dependency source pins to exercise both the MIP and NLP variants and compare objective agreement.
Reviewed changes
Copilot reviewed 10 out of 11 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
test/test_device_hvdc.jl |
Updates the lossy HVDC test to use Bin2 formulation; adds NLP test and an objective agreement check. |
test/Project.toml |
Updates [sources] URLs and removes the InfrastructureOptimizationModels source pin. |
src/operation/emulation_model.jl |
Changes warning message interpolation to use model.name. |
src/operation/decision_model.jl |
Changes warning message interpolation to use model.name. |
src/mt_hvdc_models/hvdcsystems_constructor.jl |
Refactors converter construction; adds Bin2-based model build stage and reworks QuadraticLossConverter stage to use IOM approximation helpers. |
src/mt_hvdc_models/HVDCsystems.jl |
Generalizes quadratic-loss converter support; adds helper functions and updates loss/current constraints to use new expressions. |
src/core/variables.jl |
Removes legacy HVDC squared/aux/interpolation variable type definitions. |
src/core/formulations.jl |
Adds AbstractQuadraticLossConverter and new Bin2QuadraticLossConverter; updates QuadraticLossConverter docstring/intent. |
src/core/constraints.jl |
Removes legacy converter power/McCormick/interpolation constraint type definitions. |
src/PowerOperationsModels.jl |
Updates exports to reflect removed legacy HVDC variable types; exports new converter formulation types. |
Project.toml |
Moves/updates [sources] to Sienna-Platform URLs and pins InfrastructureOptimizationModels to main; tightens compat formatting. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| logger = IS.configure_logging(get_internal(model), IOM.PROBLEM_LOG_FILENAME, file_mode) | ||
| if store_system_in_results | ||
| @warn "store_system_in_results for $(model) is set to true. This will do nothing unless a Simulation is being built." | ||
| @warn "store_system_in_results for $(model.name) is set to true. This will do nothing unless a Simulation is being built." |
combine ModelConstruct stages for both formulations only add linear loss formulations for devices with non-zero linear loss
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
* Add HVDCTwoTerminalVSC two-terminal HVDC formulation Add two new formulations for PSY.TwoTerminalVSCLine: HVDCTwoTerminalVSC (NLP) and HVDCTwoTerminalVSCBin2 (SOS2 MILP). Both model independent active/reactive power control per terminal under a PQ capability circle, quadratic+linear converter losses, and explicit cable resistance via v_f - v_t = (1/g)*I. Reactive variables/constraints are added only on full AC networks. Refactor shared converter-loss helpers (loss expression builder, abs-value decomposition, linear-loss detection) out of mt_hvdc_models into src/common_models/quadratic_converter_loss.jl so the multi-terminal and two-terminal VSC paths reuse the same primitives. Rename the ConverterPositiveCurrent / NegativeCurrent / CurrentDirection variable types to generic PositiveCurrent / NegativeCurrent / CurrentDirection. Point Project.toml at the InfrastructureOptimizationModels ac/hvdc-vsc branch (which carries the per-device-bounds API used by these formulations) and restore runtests.jl to run the full test list. * Address PR #119 review on HVDCTwoTerminalVSC - Rename Bin2 → MIP for VSC + IPC formulations and update tests - Make HVDCReactivePowerVariable parametric on From/To with const aliases - Wire HVDCReactivePower{From,To}Variable into ReactivePowerBalance on AC - Fix g==0 cable Ohm's law to force I==0 (open circuit, not v_f == v_t) - Add axis-aligned half-spaces to the MIP PQ-capability octagon - Default use_linear_loss=true for MIP and false for NLP via get_default_attributes; warn when NLP runs with use_linear_loss=true - Collapse abs-value decomposition into a single ModelConstructStage helper - Skip zero-coefficient terms in the quadratic converter loss expression - Fix docstring sign convention on HVDCVSCConverterPowerConstraint - Drop brittle log-file-regex linear-loss warning test Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * Address PR #119 Review 2 on HVDCTwoTerminalVSC - Add HVDCTwoTerminalVSCLP formulation that owns the octagonal linear outer-approximation of the per-terminal PQ disk; document the octagon geometry and its tightness tradeoffs vs the PWL path. - Refactor HVDCTwoTerminalVSC (NLP) and HVDCTwoTerminalVSCMIP (PWL) to share a single PQ-capability add_constraints! method that writes `p_sq + q_sq <= s^2` over IOM._add_quadratic_approx! expressions. - Collapse the HVDCReactivePowerVariable{From}/{To} add_to_expression methods into one parametric method via a new `_vsc_arc_bus` helper; collapse `_vsc_v_from_bounds`/`_vsc_v_to_bounds` similarly. - Dispatch `_quadratic_converter_loss_expr` on i_sq_t (QuadExpr vs AffExpr) and accumulate via JuMP.add_to_expression!; degrade to AffExpr when a == 0 even on the QuadExpr branch. - Drop the explicit-type-check MINLP warnings on both VSC and MT HVDC paths; leave brief comments in their place. - Document the use_linear_loss defaults on all four get_default_attributes methods. - Reorder construct_device! to add the abs-value decomposition variables before the constraints that consume them (PositiveCurrent / NegativeCurrent were previously requested before they were added). - Replace `filter(_has_linear_loss, devices)` with a comprehension so it works on FlattenIteratorWrapper as well as Vector. - Omit the VSCMIP/VSCLP on ACPPowerModel build tests: HiGHS cannot handle the ACP network's trigonometric branch constraints. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * Address PR #119 Review 3 on HVDCTwoTerminalVSC - Drop parametric From/To types in favor of two concrete reactive-power variable structs (HVDCReactivePowerFromVariable / ToVariable); unroll the side-parametric _vsc_* accessors into two named methods each. - Rename HVDCTwoTerminalVSCMIP -> HVDCTwoTerminalVSCMILP (forward-looking for future MINLP support) and HVDCVSCReactiveCapabilityConstraint -> HVDCVSCApparentPowerLimitConstraint with a docstring covering NLP/MILP/LP paths accurately. - Split _add_abs_value_decomposition! into separate variables (ArgumentStage) and constraints (ModelStage) helpers; move the variable-creation call to ArgumentConstructStage in both the VSC and MT converter constructors. - Move _maybe_add_reactive_power_variables! / _constraints! to a new common_models/network_conditional.jl as generic helpers parameterized by variable types and constraint type, addressing issue #120. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * Address PR #119 Review 3 follow-up on HVDCTwoTerminalVSC Drop the HVDCTwoTerminalVSCMILP formulation: the PWL surrogate for the loss model is already MILP-grade, so a separate MILP-on-disk path didn't buy anything we can solve. The LP path now carries a `use_octagon` attribute (default `true`) that toggles the four diagonal half-planes on top of the always-on axis-aligned box, so users can pick either the octagon (≤8.2% loose linear outer-approximation of the disk) or just the box without adding new formulations. Refactors driven by the review feedback: - Rename MIPQuadraticLossConverter -> MILPQuadraticLossConverter (forward-looking for potential MINLP variants). - Delete _maybe_add_pq_sq_approx! and the per-formulation _uses_pq_sq_approx trait; the p_sq/q_sq IOM expressions are now registered by _register_pq_sq_expressions!, resolved by dispatch on (formulation, network) so only HVDCTwoTerminalVSC on an AC network registers them. - Drop the VSC PSY wrappers (_vsc_v_limits_*, _vsc_rating_*, _vsc_v_bounds_*, _vsc_i_bounds) and call PSY.get_voltage_limits_* / get_rating_* directly via broadcast. - Consolidate _vsc_shared_i_max into _linear_loss_i_max with method overloads for TwoTerminalVSCLine and InterconnectingConverter. - Collapse the From/To HVDCReactivePower* add_to_expression! pair into one method dispatching on the variable type via _vsc_q_terminal_bus, and flip the coefficient from -1.0 to +1.0 (VSC injects/consumes Q freely, unlike LCC which is strictly a consumer). - Replace the runtime `if N <: AbstractActivePowerModel` guards in network_conditional.jl with explicit no-op methods dispatched on NetworkModel{<:AbstractActivePowerModel}. - Combine the two _quadratic_converter_loss_expr methods via a JuMP-type-dispatched _loss_seed helper. - Drop the i_max_getter::Function argument from _add_abs_value_decomposition_constraints!; the getter is now picked by dispatch on the device type. - Switch the four VSC add_constraints! signatures to the parameterized Union{Vector{U}, IS.FlattenIteratorWrapper{U}} where {U <: ...} form used elsewhere in the file. Tests: - Delete the HVDCTwoTerminalVSCMILP testsets and rename the MILPQuadraticLossConverter testset accordingly. - Switch the MIP-vs-NLP agreement test to LP-vs-NLP isapprox at 5% tolerance (on DCP the PQ disk is inactive, so the LP and NLP differ only by the i² loss surrogate; the SOS2 PWL can be slightly above the exact i², so a strict ordering doesn't hold). - Add a new testset that asserts use_octagon=true on the LP path gives an objective ≥ the box-only case, pinning the new attribute's semantics. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * formatting * Address PR #119 Review 4 on HVDCTwoTerminalVSC --------- Co-authored-by: Anthony Costarelli <acostare@nrel.gov> Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 17 out of 18 changed files in this pull request and generated 5 comments.
Comments suppressed due to low confidence (2)
src/common_models/quadratic_converter_loss.jl:45
- When
use_linear_lossis false, a nonzero proportional loss term is silently dropped instead of being modeled. That means any converter loss curve withb != 0is solved with understated losses (the new NLP/default paths set this flag to false), so this should reject/warn on such data or use a representation that preserves theb * |I|term.
if use_linear_loss && !iszero(b)
JuMP.add_to_expression!(expr, b, i_pos_t)
JuMP.add_to_expression!(expr, b, i_neg_t)
end
test/test_device_hvdc.jl:305
- This test uses
DCPPowerModel, which dispatches through the active-power-only no-op path for reactive-power constraints, soHVDCVSCApparentPowerLimitConstraintis never added and theuse_octagonattribute has no effect. The assertion can pass with identical models and does not exercise the octagon-vs-box behavior it is meant to cover; use an AC network/formulation or otherwise verify the constraint containers are present.
function _solve_lp(use_octagon)
sys = _generate_test_vsc_sys()
template = OperationsProblemTemplate(NetworkModel(DCPPowerModel))
Comment on lines
+583
to
+584
| i_pos_t = use_linear_loss ? i_pos_var[name, t] : nothing | ||
| i_neg_t = use_linear_loss ? i_neg_var[name, t] : nothing |
Collaborator
There was a problem hiding this comment.
Good catch. Also add a test case for this
Comment on lines
+1638
to
+1647
| for t in time_steps | ||
| i_pos_t = use_linear_loss ? i_pos_var[name, t] : nothing | ||
| i_neg_t = use_linear_loss ? i_neg_var[name, t] : nothing | ||
| loss_ft = _quadratic_converter_loss_expr( | ||
| a_f, b_f, c_f, i_sq_expr[name, t], i_pos_t, i_neg_t; | ||
| use_linear_loss = use_linear_loss, | ||
| ) | ||
| loss_tf = _quadratic_converter_loss_expr( | ||
| a_t, b_t, c_t, i_sq_expr[name, t], i_pos_t, i_neg_t; | ||
| use_linear_loss = use_linear_loss, |
Comment on lines
+392
to
+399
| Used by `HVDCTwoTerminalVSCNLP` formulations. | ||
| Docs abbreviation: ``v_f^{dc}`` | ||
| """ | ||
| struct HVDCFromDCVoltage <: VariableType end | ||
|
|
||
| """ | ||
| DC-side voltage at the to-terminal of a two-terminal HVDC link. | ||
| Used by `HVDCTwoTerminalVSCNLP` formulations. |
| # loss(I) = a * I^2 + b * |I| + c | ||
| # Used by multi-terminal InterconnectingConverter formulations | ||
| # (MILPQuadraticLossConverter, QuadraticLossConverter) and two-terminal | ||
| # HVDCTwoTerminalVSCNLP formulations. |
Comment on lines
+277
to
+278
| # ACP network's trigonometric (cos/sin) branch ohms-law constraints; we'd | ||
| # need an MINLP-capable solver (e.g. Gurobi) for that combination. |
Collaborator
luke-kiernan
left a comment
luke-kiernan
left a comment
There was a problem hiding this comment.
Took a brief look--a few small things. But you should have someone more familiar with these formulations also take a look
| time_steps = get_time_steps(container) | ||
| for d in devices | ||
| name = PSY.get_name(d) | ||
| bus_no = PNM.get_mapped_bus_number( |
Collaborator
There was a problem hiding this comment.
I'm a little surprised to see this get_mapped_bus_number call here. What's the status of our support and testing for network reductions? Problem is that anything bus-associated gets weird...
Member
Author
There was a problem hiding this comment.
LCC does something similar.
Contributor
There was a problem hiding this comment.
Is needed for network reductions I think
| Docs abbreviation: ``i^{+,dc}`` | ||
| """ | ||
| struct AuxBilinearConverterVariable <: VariableType end | ||
| struct PositiveCurrent <: VariableType end |
Collaborator
There was a problem hiding this comment.
Generic name for something with a pretty narrow use case. Call it PositiveAbsDecompCurrent instead? Oh but we like to use full words for public interface where possible...
| """ | ||
| struct AuxBilinearSquaredConverterVariable <: VariableType end | ||
|
|
||
| Negative part of an absolute-value current decomposition (``i = i^+ - i^-``). |
Collaborator
There was a problem hiding this comment.
Negative part of an absolute-value current decomposition (
i = i^+ - i^-)
Is this "absolute value decomposition" thing standard terminology? I had to go pull up _add_abs_value_decomposition_constraints! in order to figure out what's going on. Namely, only one of i^+ and i^- can be nonzero, and whichever one is nonzero equals abs(i).
| Quadratic Loss InterconnectingConverter using the separable bilinear approximation | ||
| (`v·i = ½((v+i)² − v² − i²)`) with a SOS2-based PWL approximation for x². Stays MILP. | ||
| """ | ||
| struct MILPQuadraticLossConverter <: AbstractQuadraticLossConverter end |
Collaborator
There was a problem hiding this comment.
nitpick: it'd be more consistent to put MILP at the end of the name, since we have HVDCTwoTerminalVSCNLP. Similarly I'd maybe add a MINLP to the end of QuadraticLossConverter.
Member
Author
There was a problem hiding this comment.
Will be in later issue
Comment on lines
+583
to
+584
| i_pos_t = use_linear_loss ? i_pos_var[name, t] : nothing | ||
| i_neg_t = use_linear_loss ? i_neg_var[name, t] : nothing |
Collaborator
There was a problem hiding this comment.
Good catch. Also add a test case for this
Replace the abs-value decomposition (PositiveCurrent + NegativeCurrent + CurrentDirection binary, gated by use_linear_loss attribute) with a single linear AbsoluteValueCurrent variable bounded by abs_i >= ±i. The LP relaxation pins abs_i = |i| at the optimum because the loss term b*abs_i is minimized via the generation-cost objective, so no binary or complementarity constraint is needed. With the binary gone, NLP solvers (Ipopt) can also handle the linear loss term, so use_linear_loss collapses into the default codepath and is deleted on all four formulations. _quadratic_converter_loss_expr becomes two type-dispatched methods on typeof(i_sq_t): AffExpr for the MILP/PWL path, QuadExpr for the NLP/exact bilinear path. Each method is type-stable and returns the natural expression type for its formulation. Rename MILPQuadraticLossConverter -> QuadraticLossConverterMILP and QuadraticLossConverter -> QuadraticLossConverterNLP for NLP/MILP suffix consistency. Consolidate test/test_device_hvdc.jl: drop two redundant standalone MT build+solve tests, three redundant standalone VSC build+solve tests, and the octagon test (which silently passed on DCPPowerModel because HVDCVSCApparentPowerLimitConstraint is a no-op on active-power-only networks). Add one new test that verifies AbsoluteValueCurrent ≈ |ConverterCurrent| at the MILP optimum — direct evidence that the binary-free abs-value formulation is tight. Also addresses PR #103 review comments: - Drop misleading "NLP-only" wording from HVDCFromDCVoltage / HVDCToDCVoltage docstrings. - New header docstring on quadratic_converter_loss.jl explains both NLP and LP paths. - Mixed-loss-device KeyError bug (Copilot's comment) disappears by construction: AbsoluteValueCurrent is now registered for every device. - Silent-drop bug (use_linear_loss=false dropping nonzero b) disappears too: there's no toggle and b*abs_i is always modeled. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
rodrigomha
reviewed
May 19, 2026
| ######## Absolute-value surrogate ####### | ||
| ######################################### | ||
|
|
||
| function _add_abs_value_variables!( |
Contributor
There was a problem hiding this comment.
Why do we need this method? Can we just call add_variables! directly in the constructor?
rodrigomha
reviewed
May 19, 2026
| return expr | ||
| end | ||
|
|
||
| function _quadratic_converter_loss_expr( |
Contributor
There was a problem hiding this comment.
This methods are good. We should add docstrings to explain here that we skip the terms.
Also, can we specify that abs_i_t is a JuMP something here so we avoid compiling an Any for abs_i_t?
rodrigomha
requested changes
May 19, 2026
Contributor
rodrigomha
left a comment
rodrigomha
left a comment
There was a problem hiding this comment.
This is looking good. Just minor comments and we are good to go.
Rename AbsoluteValueCurrent → CurrentAbsoluteValueVariable to match its sister CurrentAbsoluteValueConstraint. Inline the one-line _add_abs_value_variables! wrapper at its two call sites. Add a docstring to _quadratic_converter_loss_expr explaining why the iszero guards drop zero coefficient terms, and annotate abs_i_t::JuMP.VariableRef so the methods don't infer Any. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
rodrigomha
approved these changes
May 20, 2026
Contributor
rodrigomha
left a comment
rodrigomha
left a comment
There was a problem hiding this comment.
I'm happy with this if tests are passing locally
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.