-
Notifications
You must be signed in to change notification settings - Fork 150
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Adding SOC extended DistFlow (BFM) formulation #229
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like a good start. Detailed comments to follow.
src/core/variable.jl
Outdated
|
||
|
||
"variable: `0 <= i[l] <= (Imax)^2` for `l` in `branch`es" | ||
function variable_series_current_magnitude_sqr(pm::GenericPowerModel, n::Int=pm.cnw; bounded = true) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think you need this constraint. You can move this implementation into this shared variable space,
https://github.com/lanl-ansi/PowerModels.jl/blob/master/src/form/wr.jl#L399
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, great approach, hadn't noticed this before :)
src/form/wi.jl
Outdated
""" | ||
Creates Ohms constraints (yt post fix indicates that Y and T values are in rectangular form) | ||
""" | ||
function constraint_ohms_yt_from(pm::GenericPowerModel{T}, n::Int, f_bus, t_bus, f_idx, t_idx, g, b, c, tr, ti, tm) where T <: AbstractWIForm |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree this does not make a lot of sense. I think this formulation needs a new OPF formulation that better reflect its construction.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, I agree, made a proposal that still allows you to use BIM formulations in the BFM problem type and vice-versa.
src/form/wi.jl
Outdated
w_fr = pm.var[:nw][n][:w][f_bus] | ||
w_to = pm.var[:nw][n][:w][t_bus] | ||
l = f_idx[1] | ||
i = pm.var[:nw][n][:i][l] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have a look at the QC model for variable naming conventions. I have been using cm
so far.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, cm it is.
Based on your recommendation lets adopt the name BranchFlow (i.e. Because the equations in the base formulation are quite different, I think we need to add a new problem name for this variant of the model, I am thinking Concerning Concerning, the inconsistency on test case 24. My guess is that it is due to the fact that the BF formulation does not implement this constraint, https://github.com/lanl-ansi/PowerModels.jl/blob/master/src/form/wr.jl#L516 To the best of my knowledge, the constraint has never been projected into the BF space. I would be happy to do the derivation, in due course. For now I would confirm that this is the issue and then integrate the case 24 test with a different objective value. |
Codecov Report
@@ Coverage Diff @@
## master #229 +/- ##
==========================================
+ Coverage 93.53% 93.74% +0.21%
==========================================
Files 36 39 +3
Lines 4115 4335 +220
==========================================
+ Hits 3849 4064 +215
- Misses 266 271 +5
Continue to review full report at Codecov.
|
Thank you very much for your feedback. Further commits forthcoming. I'll make the Concerning I will confirm soon that the difference between the current SOC-BIM and SOC-BFM is due to the missing lifted nonlinear cuts. For now I changed the objective value in the SOC-BFM tests. |
- new problem type - code for using BFM in opf and BIM in opf_bf - new tests
Updated the naming of the current variable, now also documented here for future reference. Is this what you expect cfr #26?
The results of SOCWR and DistFlow for |
Concerning the problem name. We need a generic problem name which will then take different forms. As an example, the abstract problem I am inclined to call the abstract problem as The variable name docs looks good to me. I like the |
todo: fix repeated warnings
Ok, great, let me confirm. So, the problem types are named Currently, the convex relaxation of the (balanced) DistFlow formulation (shorthand The BFM-equivalent constraints of BIM-style
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking better, comments inline.
src/prob/opf_bf.jl
Outdated
constraint_branch_kvl(pm, i) | ||
constraint_branch_current(pm, i) | ||
|
||
#constraint_voltage_angle_difference(pm, i) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this commented out?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
forgot to remove after trying to find the source of the issue with case24 optimality BIM vs BFM
function post_opf_bf(pm::GenericPowerModel) | ||
variable_voltage(pm) | ||
variable_generation(pm) | ||
variable_branch_flow(pm) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should add branch current variables as well, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
current is also a flow variable, so I had put the initialization of the current variables at the same level as the calls to the active and reactive power initialization. will be addressed
src/prob/opf_bf.jl
Outdated
end | ||
|
||
for i in ids(pm, :branch) | ||
constraint_branch_flow_losses(pm, i) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
constraint_branch
feels redundant given that losses are only defined on branches. How about constraint_flow_losses
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok
src/prob/opf_bf.jl
Outdated
|
||
for i in ids(pm, :branch) | ||
constraint_branch_flow_losses(pm, i) | ||
constraint_branch_kvl(pm, i) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
kvl is a property of cycles on the network, correct? I don't see how it connects to this constraint. I think voltage_magnitude_difference
is accurate and consistent with the constraints already defined.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe it should refer to Ohm's law, as it relates two voltages through impedance. But that may create confusion w.r.t. BIM-style OPF problem formulation.
src/prob/opf_bf.jl
Outdated
for i in ids(pm, :branch) | ||
constraint_branch_flow_losses(pm, i) | ||
constraint_branch_kvl(pm, i) | ||
constraint_branch_current(pm, i) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is probably the exception to the rule. Here I think it makes sense to keep branch
explicit, because there are so many possibilities for different currents.
Also I will have to think about how to unify this with constraint_voltage
, but I will leave that for future issue that is outside the scope of this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok
src/prob/opf_bf.jl
Outdated
|
||
objective_min_fuel_cost(pm) | ||
|
||
constraint_voltage(pm) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is not immediately clear to me if this is needed in problem definition. It might help you to comment it out for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, removed
src/form/shared.jl
Outdated
# https://docs.julialang.org/en/release-0.6/manual/style-guide/#Avoid-strange-type-Unions-1 | ||
# and should be used with discretion. | ||
# | ||
# If you are about to add a union type, | ||
# first double check if a different type hierarchy can resolve the issue | ||
# instead. | ||
# | ||
AbstractBIMForms = Union{AbstractACPForm, AbstractACTForm, AbstractDCPForm, AbstractWRForm, AbstractWRMForm} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that type unions are discouraged in Julia. So for now, I would recommend removing this collection and simply say that the opf_bf
problem does not yet support these forms and defining new forms just for BFMs.
Later on after this PR, I will consider unification of both forms.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, unification code removed
@@ -439,11 +439,12 @@ function constraint_voltage_angle_difference(pm::GenericPowerModel, n::Int, i::I | |||
branch = ref(pm, n, :branch, i) | |||
f_bus = branch["f_bus"] | |||
t_bus = branch["t_bus"] | |||
arc_from = (i, f_bus, t_bus) | |||
pair = (f_bus, t_bus) | |||
buspair = ref(pm, n, :buspairs, pair) | |||
|
|||
if buspair["branch"] == i |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This might also be the issue of inconsistent on case 24. You might be to post this constraint for all branches in the BFM models.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the end, this was not the problem, just a typo in the DistFlow equation for constraint_voltage_angle_difference
Results are now identical between |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are close now. Proposed a few ways to further simplify things and a few questions about the math.
src/form/df.jl
Outdated
end | ||
|
||
"" | ||
function variable_branch_flow(pm::GenericPowerModel{T}, n::Int=pm.cnw; kwargs...) where T <: AbstractDFForm |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this function can be omitted. The most generic version works for all GenericPowerModels.
https://github.com/lanl-ansi/PowerModels.jl/blob/master/src/core/variable.jl#L223
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
indeed
src/form/df.jl
Outdated
end | ||
|
||
"do nothing, this model does not have complex voltage constraints" | ||
function constraint_voltage(pm::GenericPowerModel{T}, n::Int) where T <: AbstractDFForm |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This implementation can be provided without any constraint on T
and can be placed in core/constraint.jl
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok
|
||
#define series flow expressions to simplify Ohm's law | ||
p_fr_s = p_fr - g_sh_fr*(w_fr/tm^2) | ||
q_fr_s = q_fr + b_sh_fr*(w_fr/tm^2) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not immediately obvious to me that this is correct. Did you double check? Until we have an asymmetrical network test this will not be checked by unit tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The derivation goes as follows. Branches are modeled with an ideal transformer and a pi-section in series.
As the ideal transformer is located at the from side, the voltage after the transformer, i.e. at the from side of the pi-element following it, is actually (w_fr/tm^2)
. Furthermore, as this transformer is ideal, it is lossless. Moreover, as we'll be doing the DistFlow derivation, which performs the angle relaxation, transformer phase shift does not influence the power flow (except for constraint_voltage_angle_difference
.)
We observe that a a branch's power flow balance with asymmetrical shunts is composed of three loss contributions: p_fr + p_to = p_sh_loss_fr + p_s_loss + p_sh_loss_to
, q_fr + q_to = q_sh_loss_fr + q_s_loss + q_sh_loss_to
, where x_sh_loss_fr
, x_sh_loss_to
are the to from and to side shunt losses, and x_s_loss
is the series flow loss. We can further define the flow only through the series impedance as p_fr_s + p_to_s = p_s_loss
, q_fr_s + q_to_s = q_s_loss
.
The pi-section loss terms, expressed in the w
and ccm
variables therefore are:
p_sh_loss_fr = g_sh_fr*(w_fr/tm^2)
q_sh_loss_fr = -b_sh_fr*(w_fr/tm^2)
p_s = r*ccm
p_s = x*ccm
p_sh_loss_to = g_sh_to*(w_to)
q_sh_loss_to = -b_sh_to*(w_to)
Ohm's law only relates the voltage magnitude and the series current and series impedance of the pi-element. Therefore, after performing the DistFlow derivation (v_j = v_i - z_ij *I_ij_s
, with RHS and LHS multiplied by their complex conjugated) and the subsititution of the squared node voltage magnitude, (with w
and squared branch series current magnitude, with ccm
), you obtain expressions in the series complex power flow variables p_fr_s, q_fr_s
. Furthermore, in these variables p_fr_s^2 +q_fr_s^2 == (w_fr/tm^2)*ccm
is a valid expression, which is then relaxed.
I can add this somewhere in the documentation.
I haven't found the time to rederive constraint_voltage_angle_difference
to confirm its current implementation though.
|
||
#define series flow expressions to simplify constraint | ||
p_fr_s = p_fr - g_sh_fr*(w_fr/tm^2) | ||
q_fr_s = q_fr + b_sh_fr*(w_fr/tm^2) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above, it's not immediately obvious to me that this is correct. Did you double check? Especially because in my derivation the shunt value did not occur in this constraint.
|
||
|
||
"variable: `0 <= i[l] <= (Imax)^2` for `l` in `branch`es" | ||
function variable_branch_series_current_magnitude_sqr(pm::GenericPowerModel, n::Int=pm.cnw; bounded = true) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we replace this function with the one that is already defined here, https://github.com/lanl-ansi/PowerModels.jl/blob/master/src/form/wr.jl#L399? If so, that function can move to core/variable.jl because it is generally applicable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These constraints have different meaning: bounding series current in this BFM derivation vs bounding total current magnitude in QC. Anyway, I updated the series current upper bound with a compensation term for the current contribution of the shunts.
docs/src/developer.md
Outdated
|
||
## DistFlow derivation | ||
|
||
### For an asymmetric pi section |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
here's the derivation, @ccoffrin
The implementation looks reasonable. I would like to understand the math a bit better before we merge this in. Can we setup a VC to go over it? |
@frederikgeth, fixed merge conflict so that travis would run. |
@frederikgeth seems there are still a few updates needed to bring thus up to date with master. |
# Conflicts: # src/form/shared.jl
# Conflicts: # src/form/shared.jl
Thanks! |
I'm working on an implementation of the extended DistFlow formulation.
Before I continue, I'm wondering what you think of the following issues:
i
toconstraint_voltage_angle_difference(pm, n, *i*, f_bus, t_bus, buspair["angmin"], buspair["angmax"])
W
variables but not WR/WI but squared current magnitude instead. Currently it is namedAbstractWIForm
, to refer to the 'W' voltage variables and current variables 'I', but I don't think it is an ideal name.AbstractWForms = Union{AbstractWRForms, AbstractWIForm}
constraint_ohms_yt_to(pm, i)
in this formulation does nothing. Should the problem definitions be changed, where a callconstraint_ohms(pm, i)
for BIM-type models itself callsconstraint_ohms_yt_from(pm, i)
andconstraint_ohms_yt_to(pm, i)
?constraint_voltage_angle_difference
though, as the test case 24 (with SAD) returns an objective value which is too low, I'll take a deeper look hopefully sometime the coming week.any other comments are welcome of course :)