WIP: SDDiP

[lkapelevich]

• handle integer state variables (bug somewhere atm)
• handle continuous state variables
• remove all the ext[:issddip]=true and find a nice way to distinguish for a node/subproblem that we want Lagrangian duals (half done)
• improve the code for calculating duals, better initial values, get rid of extra vectors being created
• remove manifest
• move over remaining examples
• pass the SDDP model's solver through to Kelley's method
• remove the speed stuff from the binary expansion code, unnecessary
• raise an issue: state variables are assumed to be nonnegative here just mentioned in docstrings
• allow user to set epsilon in continuous state variables
  closes SDDiP #195 and Complete rewrite lkapelevich/SDDiP.jl#21

[odow]

handle integer state variables (bug somewhere atm)
handle continuous state variables

@variable(sp, x, SDDP.SDDiP.BinaryState(), initial_value = 1.0)
@variable(sp, 0 <= x <= 10, SDDP.SDDiP.ContinuousState(), initial_value = 1.0)
@variable(sp, 0 <= x <= 10, SDDP.SDDiP.IntegerState(), initial_value = 1.0)

remove all the ext[:issddip]=true and find a nice way to distinguish for a node/subproblem that we want Lagrangian duals

See previous comment. Make the new JuMP variable types and set :issddip in add_variable.

Bonus points for including a way to check for SDDP.State variables that have been added and throw an error if :issddip is set.

improve the code for calculating duals, better initial values, get rid of extra vectors being created

👍

remove manifest

Why is it needed at the moment?

move over remaining examples

More the merrier.

[lkapelevich]

Why is it needed at the moment?

It's not, I pushed it without realizing

[lkapelevich]

So by the SDDP.SDDiP.ContinuousState(), I take it you imagining this being a plugin/submodule? I think everything here can easily be moved, except the most invasive part here:

SDDP.jl/src/algorithm.jl

Line 167 in 67378f2

if !node.ext[:issddip]

which relies on SDDP knowing about SDDiP, and I couldn't think of how to get this working without changing code algorithm.jl, or else parameterizing Node. So would you parameterize Node by the type of states it has (SDDP vs SDDiP)?

There is also minor gotcha here

SDDP.jl/src/algorithm.jl

Line 868 in 67378f2

if !haskey(model.nodes[1].ext, :issddip)

.
but basically we just need to tell a Node whether it will be involved in SDDiP.

[odow]

It doesn't have to be a submodule. You could also go SDDP.SDDiPState(). But having almost everything self-contained in a single file would be nice.

I don't really want to parameterize Node. Instead, one argument for including SDDiP in the package is so that we can have tighter interactions like you describe instead of splitting everything into lots of different packages. That said, you might be able to make an over-loadable version of get_dual_variables somehow.

[lkapelevich]

After a think, I don't think there is a reason to introduce SDDiPState or similar unless you really like it as the interface. A user will either use SDDiP, in which case all variables need to be binary-fied, or they will not. So it's easier to just have a single switch for any state variables added, a keyword argument SDDiP = true in PolicyGraph will do everything necessary I think.

[odow]

Is it possible to add the keyword to train? Then the default remains as-is, and we can solve the LP relaxation. You could even pass in a type, add it to Options and then use it to dispatch on any methods you need to overload.

abstract type AbstractSMIPSolver end
struct ContinuousRelaxation <: AbstractSMIPSolver end
struct SDDiP <: AbstractSMIPSolver end
struct MyCoolNewSolver <: AbstractSMIPSolver end

SDDP.train(model, integrality_method = SDDP.SDDiP())

(Edit: although, then we get in trouble if you try to solve multiple times. Maybe an argument to PolicyGraph is best.)

[lkapelevich]

Oops I pushed before reading the comment

Edit: I'll think about the keyword to train

[lkapelevich]

What exactly is the problem you see if we try to solve multiple times? If the keyword is added to train then the approach would be to not modify add_variable but instead in train, loop over all nodes and add new binary variables, then proceed with sddp. We would need to flag nodes once the extra variables have been added so that if we train multiple times the variables don't get added again. Were you thinking of something else?

[odow]

Basically I would worry that someone would go

SDDP.train(model, integrality_method = SDDP.SDDiP(), iteration_limit = 10)
SDDP.train(model, integrality_method = SDDP.ContinuousRelaxation(), iteration_limit = 10)

Would anything problematic happen the second time?

[lkapelevich]

Yes this is actually worse than what I was thinking, because the first train would add a bunch of new state variables that we don't want for the second train.

An argument to PolicyGraph is probably best.

[odow]

Looking good @lkapelevich. Modulo the test failures. But I guess you're on those.

[codecov]

Codecov Report

Merging #237 into master will increase coverage by 0.25%.
The diff coverage is 96.73%.

@@            Coverage Diff             @@
##           master     #237      +/-   ##
==========================================
+ Coverage   93.82%   94.07%   +0.25%     
==========================================
  Files          14       17       +3     
  Lines        1231     1351     +120     
==========================================
+ Hits         1155     1271     +116     
- Misses         76       80       +4

Impacted Files	Coverage Δ
src/SDDP.jl	100% <ø> (ø)	⬆️
src/algorithm.jl	97.85% <100%> (+0.01%)	⬆️
src/user_interface.jl	97.52% <100%> (+0.25%)	⬆️
src/binary_expansion.jl	100% <100%> (ø)
src/plugins/headers.jl	83.33% <50%> (-16.67%)	⬇️
src/JuMP.jl	90.9% <90.9%> (ø)
src/plugins/integrality_handlers.jl	97.26% <97.26%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 12ef874...fd99920. Read the comment docs.

[odow]

Looks like you forgot to commit src/JuMP.jl.

[odow]

Great! The doc-test failures aren't your fault. The other PR's are also failing. I'm trying to fix it.

[odow]

If you rebase onto the latest master, the documentation should be fixed.

[lkapelevich]

@odow anything else?

[odow]

Great @lkapelevich!

The only other thing is some docs. If you have time, great. If you don't, I'll merge this as-is and add a TODO for some docs.

[lkapelevich]

I don't want to spend time writing docs right now, but happy to answer questions for docs.