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

Lp sensitivity summary #1917

Merged
merged 15 commits into from
Apr 25, 2019
Merged

Conversation

EdvinAblad
Copy link
Contributor

Adds two functionalities:
perturbation_range_of_feasibility(constraint)
perturbation_range_of_optimality(variable)
(Targets the issue #1332.)

The choice to present the perturbation (differences) rather than the actual ranges is mainly because the rhs is not necessarily a constant, related to #1890. (It seems sane that these two functions have consistent output).
Are the names too verbose? is e.g., range_of_feasibility better?

The functions can be optimized and probably simplified, currently the Lower/UpperBoundRef is not used due to #1892.

The perturbation_range_of_feasibility is implemented for affine and variable constraints. However, the interval sets are intentionally not supported (the interpretation of shifting the entire interval seems far-fetched).

Maybe all sensitivity analysis (possibly also shadow_price) should be moved from constraints.jl, this since they are closely connected (using the same internal functions) and in particular perturbation_range_of_optimality feels out of place. To where?

Tests to check these functions are also added. The Clp solver (jump-dev/Clp.jl#54) was used to test some larger instances (everything seems fine).

Documentation is intentionally postponed until e.g., the desired output, names, etc. are decided.

A last note, these functions can be heavily optimized if vectorized, i.e., if the ranges of all/many coefficients are desired. So it can be implemented if needed.

Copy link
Member

@odow odow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good stuff!

I wonder if converting to standard form can be greatly simplified using a MOIU Model and bridges.

Basically, define new model like

MOIU.@model(MyModel,
    (),
    (MOI.Interval,),
    (),
    (),
    (MOI.SingleVariable,),
    (),
    (MOI.Zeros,),
    (MOI.VectorAffineFunction)
)

and then MOI.copy_to(my_model, JuMP.backend(model)) (you might need to use some automatic bridges). Then this would automatically convert the problem to standard form.

There are quite a few style things: http://www.juliaopt.org/JuMP.jl/latest/style/. In particular, prefer long, explicit names over short abbreviations.

I don't think we're too concerned about performance since this is usually just a one-off.

s.t. Ax == 0
L <= x <= U
"""
function _std_matrix(model::Model)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with Oscar, this should be done by defining the standard form as a MOIU model and letting bridges do the transformation during copy_to

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That would indeed be neat. However, I can't get it to work, I might be missing some thing (not so used with automatic bridges...)

Main issue: Every constraint should get an associated slack variable.
My idea was to copy the model to a model having only interval constraints, (Since you can create an MOI.Interval, from other the other sets.) However, I always get an unsupported constraint error. Moreover, I can't find a bridge that creates intervals (only the ones that splits intervals).

If this was accomplished then the slack bridge would accomplish the rest (I think).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No there is not any bridge that transforms GreaterThan or LessThan to Interval yet. You can try with

MOIU.@model(MyModel,
    (),
    (MOI.Interval, MOI.GreaterThan, MOI.LessThan, MOI.EqualTo),
    (),
    (),
    (MOI.SingleVariable,),
    (),
    (MOI.Zeros,),
    (MOI.VectorAffineFunction)
)

@codecov
Copy link

codecov bot commented Mar 30, 2019

Codecov Report

Merging #1917 into master will increase coverage by 2.77%.
The diff coverage is 95.78%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1917      +/-   ##
==========================================
+ Coverage   88.51%   91.28%   +2.77%     
==========================================
  Files          32       32              
  Lines        4059     4110      +51     
==========================================
+ Hits         3593     3752     +159     
+ Misses        466      358     -108
Impacted Files Coverage Δ
src/constraints.jl 93.63% <95.78%> (+6.75%) ⬆️
src/parse_nlp.jl 90.06% <0%> (+0.59%) ⬆️
src/operators.jl 84.04% <0%> (+0.64%) ⬆️
src/nlp.jl 91.26% <0%> (+1.65%) ⬆️
src/_Derivatives/conversion.jl 98.07% <0%> (+1.85%) ⬆️
src/JuMP.jl 84.92% <0%> (+1.97%) ⬆️
src/Containers/SparseAxisArray.jl 81.57% <0%> (+2.09%) ⬆️
src/aff_expr.jl 92.56% <0%> (+2.23%) ⬆️
src/macros.jl 92.94% <0%> (+2.31%) ⬆️
src/_Derivatives/sparsity.jl 97.46% <0%> (+2.4%) ⬆️
... and 10 more

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 c018029...26433d6. Read the comment docs.

@codecov
Copy link

codecov bot commented Mar 30, 2019

Codecov Report

Merging #1917 into master will increase coverage by 2.65%.
The diff coverage is 93.97%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1917      +/-   ##
==========================================
+ Coverage   88.51%   91.17%   +2.65%     
==========================================
  Files          32       33       +1     
  Lines        4059     4145      +86     
==========================================
+ Hits         3593     3779     +186     
+ Misses        466      366     -100
Impacted Files Coverage Δ
src/JuMP.jl 84.09% <0%> (+1.14%) ⬆️
src/lp_sensitivity.jl 94.54% <94.54%> (ø)
src/aff_expr.jl 88.09% <0%> (-2.23%) ⬇️
src/variables.jl 97.71% <0%> (+0.03%) ⬆️
src/objective.jl 95.23% <0%> (+0.23%) ⬆️
src/operators.jl 83.98% <0%> (+0.58%) ⬆️
src/parse_nlp.jl 90.06% <0%> (+0.59%) ⬆️
src/nlp.jl 91.26% <0%> (+1.65%) ⬆️
... and 14 more

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 c018029...d23e414. Read the comment docs.

@EdvinAblad
Copy link
Contributor Author

EdvinAblad commented Mar 30, 2019

Revised according to most of your comments.
The simplification of _std_matrix still remains (I did some minor simplifications).
Have also realized that this solution currently don't support models containing vectorized constraints, this could probably be resolved by revising _std_matrix...
Also the tolerance issue should probably be targeted.

Also, (as you noted) I tend to use abbreviations, e.g., var for variable.
Since I think it makes the code more readable, but I really don't care, if you want verbose, I revise again.

Thank you for your feedback =) It's very appreciated.

Also why do the 32bit version fail to build?

end
UmX_B = U[basic] - x_B
LmX_B = L[basic] - x_B
pos = rho .> 1e-7
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would argue that we should stick to the textbook version and warn if we detect possible numerical instability. Otherwise, these numerical constants need a justification.

Copy link
Contributor Author

@EdvinAblad EdvinAblad Apr 4, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would claim that this 1e-7 is the textbook version =)
C.f. "Computational Techniques of the Simplex Method" by István Maros section 9.3.4.
Out of context but this regards the values for the feasibility range:

In the latter case a typical values are 1e-8 <= e_f <= 1e-6 if
standard double precision arithmetic with 53 bit mantissa is used.

I could double check what notes he does about other choices or correct the current one (I think he applies the tolerance in the numerator) =)

I,e., I really don't think using 0.0 as tolerance is wise (it might not work even for some simple problems), is one justification a reference to this textbook?

@mlubin
Copy link
Member

mlubin commented Apr 4, 2019

Also why do the 32bit version fail to build?

The error is MethodError: no method matching lu!(::SparseMatrixCSC{Float64,Int64}, ::Val{true}; check=true) and if you dig into the stack trace you see,

 [7] lp_rhs_perturbation_range(::ConstraintRef{Model,MathOptInterface.ConstraintIndex{MathOptInterface.ScalarAffineFunction{Float64},MathOptInterface.EqualTo{Float64}},ScalarShape}) at C:\projects\jump-jl\src\lp_sensitivity.jl:203

Because Julia doesn't follow JuMP's style recommendation "A user should see a MethodError only for methods that they called directly.", it's on you to infer which internal assumption of backslash is violated. I would guess that you need to use SparseMatrixCSC{Float64,Int32} on a 32-bit system.

@EdvinAblad
Copy link
Contributor Author

EdvinAblad commented Apr 6, 2019

Regarding the tolerances used:

  • These are the primal/dual feasibility tolerances.
  • Without them the sensitivity analysis is likely to frequently fail since, e.g., solvers has a tolerance for <= constraints, allowing them to be slightly violated.
  • Now I use the smallest suggested (1e-8) this allow a slightly wider range of applicable models. But might return too narrow bounds. (C.f. "Computational Techniques of the Simplex Method" by István Maros section 9.3.4)

Some alternative remedies:

  • Solvers has these as parameters, e.g., Clp has PrimalTolerance and DualTolerance, could it be possible to get these? Note that the default values varies among solvers, e.g., Clp use 1e-7 and Gurobi use 1e-6.
  • Since these are model dependent, the best option could be to expose them to the user. (As an optional argument with a sane default value).

Regarding getting the input (constraint matrix, variable bounds, constraint bounds):

  • Currently, Vectorized constraints are yet not supported in the lp_sensitivity. Moreover, should the ConstraintBasisStatus for vectorized constraint be implemented in each solver interface or could e.g., LQOI do some translation. (Are vectorized constraints necessary to support atm? e.g., shadow_price do not yet support them)
  • I'm getting increasingly unsure of how the copy_to would simplify the code... If we can't transform it to only interval constraints then it feels like there will be as much code needed to loop through the new transformed problem... (Of course it could be used to generalize the procedure, if e.g., there would be a "unvectorize bridge")

Again, thanks for the feedback =)

Copy link
Member

@odow odow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Almost there! Just a couple more things.

Copy link
Member

@mlubin mlubin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for hanging in there. Just a few minor things. Please also add these functions to an appropriate spot in the JuMP documentation.

question how much an objective coefficient or standard form rhs coefficient
(c.f., [`standard_form_rhs`](@ref)) can change without violating primal or dual
feasibility of the basic solution.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add a note that this form of sensitivity analysis appears often in textbooks but has limited use in practice because optimal bases are often degenerate.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(If you object to this characterization, I'd be interested to hear the argument.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True, it depends quite a lot on the instance if these ranges are practically useful, I added a paragraph discussing briefly their limitations.

@mlubin mlubin merged commit 94e2cbb into jump-dev:master Apr 25, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants