-
Notifications
You must be signed in to change notification settings - Fork 43
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
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. |
@odow following up to see if you have been able to get any better understanding of what's going on? |
Not yet. I'll try get to it this week. |
@odow Following up to see if there are any updates on this issue? |
Taking a look now |
Okaaaay. This is a bug somewhere on our side. julia> using JuMP, Ipopt
julia> function main(flag::Bool)
model = Model(Ipopt.Optimizer)
set_silent(model)
@variable(model, u[i = 1:3] == 0.3 * i)
@variable(model, v[i = 1:3] == 0.5 * i)
@variable(model, x[i = 1:3] == 0.7 * i)
@variable(model, y[i = 1:3] == 1.1 * i)
@expression(model, pg[i in 1:3], @force_nonlinear((x[i] * u[i]) + (y[i] * v[i])))
@expression(model, pg2, 0.5 * sum(pg[i] for i in 1:3))
@expression(model, pg3, -0.5 * sum(-pg[i] for i in 1:3))
if flag
@objective(model, Min, pg2)
else
@objective(model, Min, pg3)
end
optimize!(model)
return objective_value(model)
end
main (generic function with 1 method)
julia> main(true)
5.32
julia> main(false)
10.26 Assuming that I'll keep digging. |
Okay, we're modifying existing expressions: julia> using JuMP
julia> model = Model();
julia> @variable(model, x[1:2]);
julia> y = [NonlinearExpr(:+, Any[xi]) for xi in x]
2-element Vector{NonlinearExpr}:
+(x[1])
+(x[2])
julia> a = @expression(model, sum(y[i] for i in 1:2))
x[1] + x[2]
julia> y
2-element Vector{NonlinearExpr}:
x[1] + x[2]
+(x[2]) |
I've opened an issue jump-dev/JuMP.jl#3825. I'll get this fixed ASAP since it is a pretty severe correctness bug. |
We can close this PR in favor of #454
The fix was jump-dev/JuMP.jl#3826. I'll tag a new JuMP version in jump-dev/JuMP.jl#3827, and then I'll set that as the minimum version in the compat for this package. |
I'm sorry for not chasing this down earlier. I kinda assumed that the fault was somewhere in this package! |
Closed in favor of #454 |
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