-
Notifications
You must be signed in to change notification settings - Fork 41
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
UPD: JuMP v1.15 #459
base: main
Are you sure you want to change the base?
UPD: JuMP v1.15 #459
Conversation
src/core/objective.jl
Outdated
pg = var(pm, n, :pg, i) | ||
pg = isa(pg, JuMP.Containers.DenseAxisArray) ? pg.data : pg | ||
|
||
int_dim = length(pg) | ||
if length(gen["cost"]) == 1 | ||
gen_cost[(n,i)] = gen["cost"][1] | ||
elseif length(gen["cost"]) == 2 | ||
gen_cost[(n,i)] = JuMP.@NLexpression(pm.model, gen["cost"][1]*sum(pg[i] for i in 1:int_dim) + gen["cost"][2]) | ||
gen_cost[(n,i)] = JuMP.@expression(pm.model, (-gen["cost"][1]*sum(-pg[i] for i in 1:int_dim)) + gen["cost"][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.
This should really not be needed. Let me take a look through the rest of the changes. Presumably there are now two signs that cancel each other 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.
It is very strange, we don't really understand why these are needed, but they fix a lot of the failing tests. The signs cancel out here, so this should be the exact same expression mathematically, but it affects the stability of the tests for some reason. Obviously, all of these tests pass with the old NLexpression syntax.
I have reduced the changes to as few as I could to get the tests to resolve. Below is a summary of what was resolved by which changes. Objective expressionsI added minus signs in front of gen_cost and pg for the linear terms in the objectives in the following functions:
I guess only changes to These changes fixed the following testsets (9 tests total):
Power/Current balance constraintsI added minus signs in front of summations of generator power/current and inside the summation in the following functions:
Strictly speaking only changes to These changes fixed the following testsets (4 tests total):
Load constraintsI added cancelling minus signs to expressions in the following functions:
These changes fixed the following testsets (10 tests total):
|
Note that I am not yet sure exactly the problem with the |
Okay, let me take a much deeper look here. There's obviously something weird going on. We should definitely hold off merging until we understand what. |
Updates to support JuMP v1.15 nonlinear syntax.
@odow @ccoffrin We had to make some strange changes to the expressions and constraints in order to fix the failing tests from #454. In particular, if you look at something like any of the
constraint_mc_power_balance
functions, we had to put a negative sign inside thesum
calls. By applying this to power or current variables we were able to get tests to pass. See for example line 151 in acp.jl or line 288 in objective.jl.Any ideas on why this works? We want to hold off on merging if we can until we have a grasp on the reason behind this.
CC @juanjospina