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

Add value for constraints with a custom var_value function #2317

Merged
merged 10 commits into from
Aug 31, 2020

Conversation

dourouc05
Copy link
Contributor

This proposes a link between constraints and the value method that takes a var_value argument (

JuMP.jl/src/aff_expr.jl

Lines 149 to 162 in e2fd3eb

"""
value(ex::GenericAffExpr, var_value::Function)
Evaluate `ex` using `var_value(v)` as the value for each variable `v`.
"""
function value(ex::GenericAffExpr{T, V}, var_value::Function) where {T, V}
S = Base.promote_op(var_value, V)
U = Base.promote_op(*, T, S)
ret = convert(U, ex.constant)
for (var, coef) in ex.terms
ret += coef * var_value(var)
end
ret
end
).

I was surprised that this was not supported; maybe there was a reason why? (It's the reason why I did not bother adding documentation or tests for now.)

@odow
Copy link
Member

odow commented Aug 20, 2020

I think the reason being it isn't that hard to do

obj = constraint_object(c)
value(obj.f, var_value)

What were you trying to do that led to this?

@dourouc05
Copy link
Contributor Author

But why is it so complicated, while you can do value(con_ref) for the current solution? It's just inconsistent. (The solution you're showing is only easy when you are familiar with JuMP's internals, i.e. not for a typical user.)

I'm in the process of rebuilding a part of the constraint matrix for tight constraints. I think there are several blocks of that that might go into JuMP (is_tight and coefficients, for instance, possibly with other names). Would you welcome PRs for these?

@codecov
Copy link

codecov bot commented Aug 20, 2020

Codecov Report

Merging #2317 into master will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #2317   +/-   ##
=======================================
  Coverage   91.32%   91.33%           
=======================================
  Files          42       42           
  Lines        4255     4258    +3     
=======================================
+ Hits         3886     3889    +3     
  Misses        369      369           
Impacted Files Coverage Δ
src/constraints.jl 90.51% <100.00%> (+0.14%) ⬆️
src/variables.jl 96.83% <100.00%> (+0.01%) ⬆️

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 6e329c8...f24a868. Read the comment docs.

@odow
Copy link
Member

odow commented Aug 21, 2020

It's just inconsistent

Fair point.

Do you have a concrete example of what this would be used for?

@dourouc05
Copy link
Contributor Author

I see two main uses:

  • Checking whether a constraint is tight for a new solution (my current use case).
  • Checking whether a heuristic solution respects all constraints, including those that are not supposed to be handled by the heuristic algorithm. This may also include the case of checking the quality of an initial solution, before feeding it into the solver.

@dourouc05
Copy link
Contributor Author

dourouc05 commented Aug 21, 2020

Shall I include code like this in this PR or in a new one, or would you rather not see this in JuMP? (It's very rough right now.) The major goal is to make this kind of code much easier to write (getting the constant from a constraint reference is not straightforward, you have to lose yourself for quite some time in the code — for instance, I could not ask even master's students to find it out for mandatory coursework) and to understand from the user perspective. It's not really new functionality for JuMP, though. In a sense, it's similar to sensitivity analysis: you could already do it before the PRs, but it became very easy to do.

function is_tight(con_ref::ConstraintRef{Model, <:_MOICON}, var_value::Function; ε::Float64=1.0e-6)
	lhs = value(con_ref, var_value)
	rhs = MOI.constant(moi_set(constraint_object(con_ref))) # Only for LessThan/GreaterThan
	return abs(lhs - rhs) <= ε # Only for "standard" constraints, not things from constraint programming, of course
end

function coefficients(con_ref::ConstraintRef{Model, <:_MOICON})
    nvars = MOI.get(m, MOI.NumberOfVariables())
    v = spzeros(nvars) # Or a matrix for several constraints. Sparse works well for me, but not necessarily for others… 
    for (var, coeff) in jump_function(constraint_object(con_ref)).terms # Only for AffExpr
        v[var_to_idx[var]] = coeff
    end
    return v
end

@odow
Copy link
Member

odow commented Aug 21, 2020

Okay. Plugging in new solutions to evaluate a constraint is a reasonable use-case. I can see this being useful for callbacks and future feasibility checkers.

Note that the constraint functions can also be vector-valued, so we probably need to ensure value works for those cases, and reshape the result as appropriate.

The is_tight is a gigantic can of worms that I would prefer to delay for a little bit (Here is the backstory: jump-dev/MathOptInterface.jl#1023). See also https://github.com/matbesancon/MathOptSetDistances.jl

@dourouc05
Copy link
Contributor Author

So, for is_tight and others, I'll be planning a package to host them for now (something like ´JuMPFeasibility`, as it works a lot at JuMP level?).

I'm improving this PR with documentation, tests, and genericity.

@dourouc05
Copy link
Contributor Author

dourouc05 commented Aug 25, 2020

I have just added a few tests for this functionality, including support for vectors of expressions. Is the current implementation OK? I'm wondering about value(ex::Vector{GenericAffExpr{T, V}}, var_value::Function), because I'm just broadcasting over the argument: may this have strange consequences? Or should value(con_ref::ConstraintRef{Model, <:_MOICON}, var_value::Function) do some dispatch on the type of the constraint's function?

@dourouc05
Copy link
Contributor Author

dourouc05 commented Aug 25, 2020

I also have created the package for the functions you would rather see them ripen before inclusion: https://github.com/dourouc05/JuMPFeasibility.jl (tests pass locally, I don't have the bandwidth right now to configure anything online…). I think it would a rather simple extension to check for feasibility of a solution, which could solve #693, just by looping over the constraints (if linear algebra becomes a performance problem, the code could also be extended to generate a constraint matrix to bypass one call to value per constraints). Would that be a thing to pursue?

src/aff_expr.jl Outdated Show resolved Hide resolved
src/constraints.jl Outdated Show resolved Hide resolved
@odow
Copy link
Member

odow commented Aug 25, 2020

I think it would a rather simple extension to check for feasibility of a solution, which could solve #693, just by looping over the constraints

This is the idea of MathOptSetDistances, which generalizes your is_satisfied functions.

@dourouc05
Copy link
Contributor Author

I've updated the code in this PR, thanks for your comments!

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.

Looks good, just needs a docstring.

src/constraints.jl Show resolved Hide resolved
@dourouc05
Copy link
Contributor Author

Better this way? I made a mix between value(ex::GenericAffExpr{T, V}, var_value::Function) and value(con_ref::ConstraintRef{Model, <:_MOICON}; result::Int = 1).

src/constraints.jl Outdated Show resolved Hide resolved
@odow odow merged commit 5023f14 into jump-dev:master Aug 31, 2020
@dourouc05 dourouc05 deleted the patch-5 branch August 31, 2020 00:13
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.

3 participants