fix broken SDP relaxations #262
Closed
Conversation
This file contains 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
|
ready for review/merge if travis finishes successfully |
Codecov Report
@@ Coverage Diff @@
## master #262 +/- ##
===========================================
+ Coverage 68.71% 79.06% +10.35%
===========================================
Files 38 37 -1
Lines 5268 5255 -13
===========================================
+ Hits 3620 4155 +535
+ Misses 1648 1100 -548
Continue to review full report at Codecov.
|
pseudocubic
reviewed
Apr 21, 2020
Comment on lines
+59
to
+81
| function case3_unbalanced_with_bounds_pf() | ||
| file = "../test/data/opendss/case3_unbalanced.dss" | ||
| data = PowerModelsDistribution.parse_file(file) | ||
| data["gen"]["1"]["cost"] = 1000*data["gen"]["1"]["cost"] | ||
| data["gen"]["1"]["pmin"] = 0*[1.0, 1.0, 1.0] | ||
| data["gen"]["1"]["pmax"] = 10*[1.0, 1.0, 1.0] | ||
| data["gen"]["1"]["qmin"] = -10*[1.0, 1.0, 1.0] | ||
| data["gen"]["1"]["qmax"] = 10*[1.0, 1.0, 1.0] | ||
|
|
||
| data["bus"]["1"]["bus_type"] = 3 | ||
| data["bus"]["1"]["vm"] = data["bus"]["4"]["vm"] | ||
| data["bus"]["1"]["vmin"] = data["bus"]["4"]["vmin"] | ||
| data["bus"]["1"]["vmax"] = data["bus"]["4"]["vmax"] | ||
| delete!(data["branch"], "3") | ||
| delete!(data["bus"], "4") | ||
| data["gen"]["1"]["gen_bus"] = 1 | ||
|
|
||
| for (n, branch) in (data["branch"]) | ||
| branch["rate_a"] = [10.0, 10.0, 10.0] | ||
| end | ||
|
|
||
| return data | ||
| end |
There was a problem hiding this comment.
I don't think you need to define this as a function, just put it here in the outer test set context and the sub-testsets should all be able to use data.
It doesn't look like data gets altered below in any of the tests, so it would be better to avoid multiple parses of the same file
| @@ -13,23 +13,38 @@ end | |||
| "" | |||
| function build_mc_pf_bf(pm::_PMs.AbstractPowerModel) | |||
| # Variables | |||
| variable_mc_voltage(pm; bounded=false) | |||
| variable_mc_voltage(pm; bounded=true) #TODO should be false | |||
There was a problem hiding this comment.
Is this issue still outstanding for this PR, or do you plan to address it later? What is the reason you have to set bounded=true here?
Comment on lines
+27
to
+31
| if typeof(pm)<:LPUBFDiagPowerModel | ||
| #do nothing, adding theta_ref would lead to redundant constraints | ||
| else | ||
| constraint_mc_theta_ref(pm, i) | ||
| end |
There was a problem hiding this comment.
I would recommend condensing this into a single if rather than if-else, e.g.
if !(typeof(pm)<:LPUBFDiagPowerModel)
constraint_mc_theta_ref(pm, i)
end
Comment on lines
+12
to
+20
| #enforce PSDness of leaf nodes that are not captured otherwise | ||
| for nw in _PMs.nw_ids(pm) | ||
| allbuses = Set(_PMs.ids(pm, nw, :bus)) | ||
| startingbuses = Set(i for (l,i,j) in _PMs.ref(pm, nw, :arcs_from)) | ||
| leafnodes = setdiff(allbuses, startingbuses) | ||
| for i in leafnodes | ||
| constraint_mc_voltage_psd(pm, nw, i) | ||
| end | ||
| end |
There was a problem hiding this comment.
I think the loop over nw should happen in the problem definition, should it not?
Comment on lines
+88
to
+90
| for (id, L) in Lr | ||
| JuMP.set_start_value.(LinearAlgebra.diag(Lr[id]), 0.01) | ||
| end |
There was a problem hiding this comment.
Is there a reason you use Lr[id] instead of L from the loop definition?
| for (id, gen) in _PMs.ref(pm, nw, :gen) | ||
| bus = _PMs.ref(pm, nw, :bus, gen["gen_bus"]) | ||
| vmax = bus["vmax"] | ||
| vmax = get(bus, "vmax", fill(Inf, 3)) |
There was a problem hiding this comment.
We probably don't want to hard-code 3 conductors in here, maybe refer to pm.data["conductors"]? That way it will be easier to find it later
| v_start = exp.((im*2*pi/3).*[0; -1; 1]) #TODO this should be made more generic eventually | ||
| W_start = v_start*v_start' | ||
| for (id,_) in Wr | ||
| for i in 1:3 |
There was a problem hiding this comment.
I think we don't want to hard-code 3 conductors in, maybe use the size of the matrix?
| pmax = get(gen, "pmax", fill( Inf, 3)) | ||
| qmin = get(gen, "qmin", fill(-Inf, 3)) | ||
| qmax = get(gen, "qmax", fill( Inf, 3)) | ||
| for c in 1:3 |
There was a problem hiding this comment.
I think it would be better to use conductor_ids here
| @@ -329,45 +371,45 @@ function variable_mc_load(pm::SDPUBFKCLMXModel; nw=pm.cnw) | |||
| end | |||
|
|
|||
|
|
|||
|
|
|||
pseudocubic
added a commit
that referenced
this pull request
May 4, 2020
Fixes broken SDP implementations submitted originally by @frederikgeth ADD: default bounds from OpenDSS ADD: error message for dy transformer in LPUBFModels UPD: Changelog FIX: JuMP.set_()_bound -> set_()_bound to avoid NaNs UPD: delta_gen tests -> use eng data model Closes #262
|
I implemented these fixes into the data-model branch, so they will appear in the next version thanks for the contribution @frederikgeth ! |
pseudocubic
added a commit
that referenced
this pull request
May 13, 2020
This PR contains the result of a refactor of the default user-facing data model in PowerModelsDistribution to a new `ENGINEERING` data model
A merge of this PR will result in the v0.9.0 release of PowerModelsDistribution
# What is the New Data Model
We have designed a data model that more closely matches the engineering realities of a distribution network, that allows for easy transformation and visualization of the case and includes more component types than are represented in the original (and still existing under the hood) `MATHEMATICAL` data model with which previous users will be familiar.
The new data model is documented in detail in the documentation, including all of the currently supported component types are their individual fields, the types of values, and any defaults we might assume.
# Why Do We Need a New Data Model?
In distribution networks the variety of components in a standard network is much more diverse than in a traditional transmission network, and their mathematical representations are comparatively complex. For example, in order to represent n-winding multiphase transformers with loss models we have to represent a single transformer with a large number of lines, buses, and lossless 2-winding transformers, which previously users have found confusing when attempting to compare their parsed network to the originating file. To simplify the user experience, and to introduce the ability to easily make transformations of the networks, we formulated the `ENGINEERING` data model.
# How Does It Work?
For the most part, the commands that you were using before will not change appreciably. `parse_file` is still the standard way to import data, `run_mc_` commands will still run problem specifications, and PowerModel types should not be different at all. The workflow for most users should be identical; although there are now some intermediate steps to building the JuMP model from the default data dictionary, these steps should be largely invisible to the user unless they want to expose them. Users should experience the following workflow by default:
`parse_file` returns `ENGINEERING` data model -> `run_mc_{}` runs optimization problem and returns result in `ENGINEERING` model format
What is happening behind the scenes is the following:
`parse_file` returns `ENGINEERING` data model -> `run_mc_{}` converts to `MATHEMATICAL` data model in per-unit, builds JuMP model, runs optimization, converts results in `MATHEMATICAL` model format to SI units and converts back to `ENGINEERING` format.
We have exposed to the user the functions that do these conversions if they wish to use them, and have provided examples in Jupyter Notebooks in the `/examples` folder that demonstrates both of these types of workflows.
# If the user experience is the same, what changed besides the default model?
There were a multitude of changes that happened in order to support the functionality for this new data model, as well as some other changes that address concerns and comments of users
- We have eliminated some problem types. This is perhaps one of the biggest changes users will notice; variants of problems like `run_mc_opf_iv` and `run_mc_opf_bf` are eliminated in favor of having users use `run_mc_opf` only, and redirecting to the appropriate problem definitions using multiple dispatch. The only variants that should continue to exist are truly unique problem specifications, _e.g._ `run_mc_opf_oltc` or `run_mc_mld`, etc.
- We have changed the names of variable and constraint functions to support PowerModels v0.17
- We have updated the solution building to use the capabilities in InfrastructureModels v0.5
- We have made updates to the mathematical models for transformers to make them more accurate
- We have added new capabilities to the DSS parser, parsing additional components such as loadshapes, xycurves, xfmrcodes
- We have squashed a lot of outstanding bugs in the DSS parser
- We have ensured that the component naming conventions, particularly for virtual components in the `MATHEMATICAL` model, are consistent across the package
- We have added helper functions to create models from the REPL or scripts, _e.g._ `Model`, `add_bus!`, etc., rather than rely solely on dss inputs (see notebook in `/examples`)
- `parse_dss` originally followed a similar parsing pattern as `parse_pti` in PowerModels, parsing component dictionaries into Vectors. Instead, we now parse into a structure that is much closer to the final data structures that users utilize, in order to make debugging and future parsing easier
- fixed broken SDP/SOC relaxations (see #262)
# Feature X that I want isn't in the new data model
Definitely make a new Github Issue. We view any additions to the data model to be additive and therefore not a breaking change, which means we can make rapid releases containing those additions without any problem.
# Related Issues
Closes #221
Closes #256
Closes #260
Closes #242
Closes #245
Closes #33
Closes #249
Closes #253
Closes #193
Closes #257
Closes #246
Closes #248
Closes #247
Closes #252
Closes #175
Closes #251
Closes #234
Closes #238
Closes #240
Closes #236
Closes #237
Closes #230
Closes #239
Closes #243
Closes #227
Closes #264
Closes #235
Closes #56
Closes #274
Co-authored-by: Sander Claeys <[email protected]>
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.
SDPUBFPowerModel, SDPLCMXUBFPowerModel, SOCNLPUBFPowerModel and SOCConicUBFPowerModel are broken on master. I added unit tests to catch regressions. The tests do take a little time. We should revisit and add more tests once there is a more manageable way to define case study OPF extensions (bounds).