Skip to content
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

Closed
wants to merge 6 commits into from
Closed

UPD: JuMP v1.15 #459

wants to merge 6 commits into from

Conversation

pseudocubic
Copy link
Collaborator

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 the sum 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

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])
Copy link
Contributor

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?

Copy link
Collaborator Author

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.

Project.toml Outdated Show resolved Hide resolved
src/form/acp.jl Outdated Show resolved Hide resolved
src/form/acp.jl Outdated Show resolved Hide resolved
pseudocubic and others added 4 commits May 9, 2024 07:05
Co-authored-by: Oscar Dowson <odow@users.noreply.github.com>
This reverts commit dd61a57.
@pseudocubic
Copy link
Collaborator Author

pseudocubic commented May 9, 2024

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 expressions

I added minus signs in front of gen_cost and pg for the linear terms in the objectives in the following functions:

  • _objective_mc_min_fuel_cost_polynomial_linquad
  • _objective_mc_min_fuel_cost_polynomial_nl
  • _objective_mc_min_fuel_cost_polynomial_nl_switch
  • objective_variable_pg_cost

I guess only changes to _objective_mc_min_fuel_cost_polynomial_nl and _objective_mc_min_fuel_cost_polynomial_linquad are strictly needed, but that's because of a lack of complete test coverage for the other functions, but I wanted the changes to be consistent.

These changes fixed the following testsets (9 tests total):

  • 2-bus diagonal ivr opf
  • 3-bus balanced ivr opf
  • 3-bus unbalanced ivr opf
  • capcontrol_ivr
  • test generator configuration
    • ACP/ACR tests

Power/Current balance constraints

I added minus signs in front of summations of generator power/current and inside the summation in the following functions:

  • acp.jl
    • constraint_mc_power_balance_slack
    • constraint_mc_power_balance_shed
    • constraint_mc_power_balance_shed
    • constraint_mc_power_balance_simple
    • constraint_mc_power_balance
    • constraint_mc_power_balance_capc
  • acr.jl
    • constraint_mc_power_balance
    • constraint_mc_power_balance_capc
    • constraint_mc_power_balance_shed
  • ivr.jl
    • constraint_mc_current_balance
    • constraint_mc_current_balance_capc

Strictly speaking only changes to constraint_mc_current_balance and constraint_mc_power_balance_capc are probably needed to pass the tests, but that's probably because of lack of full coverage on the others.

These changes fixed the following testsets (4 tests total):

  • 3w transformer ac pf center-tap
  • 3w transformer acp opf center-tap
  • 3w transformer acr opf center-tap
  • 3w transformer ivr opf center-tap

Load constraints

I added cancelling minus signs to expressions in the following functions:

  • acp.jl
    • constraint_mc_load_power_wye
    • constraint_mc_load_power_delta
  • acr.jl
    • constraint_mc_load_power_wye
    • constraint_mc_load_power_wye

These changes fixed the following testsets (10 tests total):

  • loadmodels connection variations

@pseudocubic
Copy link
Collaborator Author

Note that I am not yet sure exactly the problem with the branch power magnitude bound failing tests in Julia 1.6

@odow
Copy link
Contributor

odow commented May 9, 2024

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.

@pseudocubic
Copy link
Collaborator Author

@odow following up to see if you have been able to get any better understanding of what's going on?

@odow
Copy link
Contributor

odow commented Jul 29, 2024

Not yet. I'll try get to it this week.

@juanjospina
Copy link
Contributor

@odow Following up to see if there are any updates on this issue?

@odow
Copy link
Contributor

odow commented Sep 12, 2024

Taking a look now

@odow
Copy link
Contributor

odow commented Sep 13, 2024

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 -(-x) == x, then these should have the same objective values...

I'll keep digging.

@odow
Copy link
Contributor

odow commented Sep 13, 2024

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])

@odow
Copy link
Contributor

odow commented Sep 13, 2024

I've opened an issue jump-dev/JuMP.jl#3825. I'll get this fixed ASAP since it is a pretty severe correctness bug.

@odow
Copy link
Contributor

odow commented Sep 13, 2024

We can close this PR in favor of #454

  [8e850b90] libblastrampoline_jll v5.11.0+0
  [8e850ede] nghttp2_jll v1.52.0+1
  [3f19e933] p7zip_jll v17.4.0+2
        Info Packages marked with ⌅ have new versions available but compatibility constraints restrict them from upgrading.
Precompiling project...
  4 dependencies successfully precompiled in 42 seconds. 81 already precompiled.
     Testing Running tests...
[ Main | Info ] : running opendss parser tests
[ Main | Info ] : running misc data handling tests
[ Main | Info ] : running power flow (pf) tests
[ Main | Info ] : running branch-flow power flow (pf_bf) tests
[ Main | Info ] : running optimal power flow (opf) tests
[ Main | Info ] : running branch-flow optimal power flow (opf_bf) tests
[ Main | Info ] : running current-voltage optimal power flow (opf_iv) tests
[ Main | Info ] : running storage tests
[ Main | Info ] : running debug (pdf) tests
[ Main | Info ] : running multinetwork tests
[ Main | Info ] : running transformer tests
[ Main | Info ] : running capacitor control tests
[ Main | Info ] : running load models tests
[ Main | Info ] : running generator configuration tests
[ Main | Info ] : running matrix shunt tests
[ Main | Info ] : running minimum load delta (mld) tests
[ Main | Info ] : running data model creation and conversion tests
[ Main | Info ] : running explicit neutral opf bound tests
[ Main | Info ] : running explicit neutral power flow tests
[ Main | Info ] : running explicit neutral power flow tests with native julia power flow solver
Test Summary:           | Pass  Broken  Total     Time
PowerModelsDistribution | 1076       2   1078  4m58.3s
     Testing PowerModelsDistribution tests passed 

shell> git status
On branch od/update
Your branch is up to date with 'oscar/od/update'.

nothing to commit, working tree clean

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.

@odow
Copy link
Contributor

odow commented Sep 13, 2024

I'm sorry for not chasing this down earlier. I kinda assumed that the fault was somewhere in this package!

@pseudocubic
Copy link
Collaborator Author

Closed in favor of #454

@pseudocubic pseudocubic deleted the ref-jump-v115 branch September 18, 2024 13:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants