Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #143 +/- ##
==========================================
+ Coverage 94.96% 95.31% +0.35%
==========================================
Files 5 6 +1
Lines 1032 1302 +270
==========================================
+ Hits 980 1241 +261
- Misses 52 61 +9 ☔ View full report in Codecov by Sentry. |
| return | ||
| end | ||
|
|
||
| struct ForwardParameter <: MOI.AbstractVariableAttribute end |
There was a problem hiding this comment.
Why having a different attribute ? We could just use ForwardVariablePrimal for parameters as well
There was a problem hiding this comment.
We can consider that. On the other hand, ForwardVariablePrimal is for output sensitivity, while ForwardParameter is for input sensitivity. Having both different would be good for validation, since we gave up on names like: ForwardOutputVariablePrimal.
There was a problem hiding this comment.
You can different between input and output depending on whether it's a set or a get. Note that defining a new struct or a new function isn't so natural for an extension, it's more designed to add methods for existing ones. However, it is possible, see the hack in NLopt.
There was a problem hiding this comment.
Adding more details to my previous comment:
I find it strange that:
For parameters:
set ForwardVariablePrimal sets an input value that can be get to check which value was there.
While, for actual variables:
set ForwardVariablePrimal always errors, and get ForwardVariablePrimal only makes sense after forward_differentiate!
This was the main motivation for the new attribute.
About:
Note that defining a new struct or a new function isn't so natural for an extension, it's more designed to add methods for existing ones.
I think I did not understand completely. DiffOpt adds new structs. Also, a few solvers define new structs (like Gurobi.NumberOfObjectives, GLPK.CallbackFunction, COSMO.ADMMIterations).
There was a problem hiding this comment.
Ah! now I get it, you mean Julia extensions like NLoptMathOptInterfaceExt.jl
There was a problem hiding this comment.
Yes, I meant Julia extensions :) The workaround in NLopt works quite well but it becomes tricky when you try to include their docstring in the documentation. It was quite hard to make it work in JuliaManifolds/Manopt.jl#264 for instance.
Also, as from the MOI level, these are also variables and the set can be bridged to EqualTo, it makes sense to consider it as ForwardVariablePrimal.
Actually, what we could do it add ForwardConstraintSet that is defined for MOI.Parameter, MOI.EqualTo, MOI.LessThan, MOI.GreaterThan, MOI.Interval. I think we worked around it in DiffOpt by using the constant in the function but if you have a VariableIndex-in-S then you can't modify the function right ?
We could disallow ForwardConstraintSet for non-VariableIndex to avoid having two ways to set the same thing. Even if that's not consistent with the ConstraintFunction/ConstraintSet attributes, that's backward compatible. Or we can change this and tag v0.5 of DiffOpt.
The advantage of this design is that we can implement ForwardConstraintSet in the bridge that transforms Parameter to EqualTo so that the same user code works with both a POI-based solver and a solver using the bridge.
| model = direct_model(POI.Optimizer(DiffOpt.diff_optimizer(SCS.Optimizer))) | ||
| set_silent(model) | ||
| @variable(model, x) | ||
| @variable(model, p in MOI.Parameter(3.0)) | ||
| @constraint(model, cons, [x - 3 * p] in MOI.Zeros(1)) | ||
|
|
||
| # FIXME | ||
| @constraint(model, fake_soc, [0, 0, 0] in SecondOrderCone()) | ||
|
|
||
| @objective(model, Min, 2x) | ||
| optimize!(model) |
There was a problem hiding this comment.
@blegat , I had to add this fake SOC constraint to force it into the Conic model. Otherwise, it tries to use a quadratic model (as the model is linear) and then fails with an error about not being able to push the DiffOpt attributes through the scalarize bridge.
There was a problem hiding this comment.
Many bridges are missing in DiffOpt but it's easy to add, open an issue
There was a problem hiding this comment.
You can use DiffOpt.ModelConstructor to force it to use conic
|
I am implementing a test that I believe should work: function test_diff_projection()
num_A = 2
##### SecondOrderCone #####
_x_hat = rand(num_A)
μ = rand(num_A) * 10
Σ_12 = rand(num_A, num_A)
Σ = Σ_12 * Σ_12' + 0.1 * I
γ = 1.0
model = direct_model(POI.Optimizer(DiffOpt.diff_optimizer(SCS.Optimizer)))
set_silent(model)
@variable(model, x[1:num_A])
@variable(model, x_hat[1:num_A] in MOI.Parameter.(_x_hat))
@variable(model, norm_2)
# (x - x_hat)^T Σ^-1 (x - x_hat) <= γ
@constraint(
model,
(x - μ)' * inv(Σ) * (x - μ) <= γ,
)
# norm_2 >= ||x - x_hat||_2
@constraint(model, [norm_2; x - x_hat] in SecondOrderCone())
@objective(model, Min, norm_2)
optimize!(model)
MOI.set.(model, POI.ForwardParameter(), x_hat, ones(num_A))
DiffOpt.forward_differentiate!(model) # ERROR
#@test TBD
return
endBut I am getting an error at the stacktrace: Edit: I imagine that this is a missing bridge right ? |
|
I encountered an error while working with DiffOpt and POI. To demonstrate the problem, I created a minimal example: This creates a simple problem using an explicitly indexed constraint (
but when I declare the constraint in a non-indexed fashion, like this: I get an error when calling The error still happens if the constraint is not declared with a name ( |
|
this fails: using JuMP, DiffOpt, HiGHS
b = [1.0, 2.0]
m = Model(
() -> DiffOpt.diff_optimizer(
HiGHS.Optimizer;
with_parametric_opt_interface = true,
),
)
@variable(m, x[1:2] >= 0)
@variable(m, c[1:2] in MOI.Parameter.(b))
@constraint(m, con, x <= c)
@objective(m, Max, sum(x))
optimize!(m)
MOI.set(m, DiffOpt.ReverseVariablePrimal(), m[:x][1], 1.0)
DiffOpt.reverse_differentiate!(m)with |
|
replaced by #161 and jump-dev/DiffOpt.jl#262 |
4 participants
@andrewrosemberg motivated me.
I love how well layers can play with each other.
This will not be merged (as part of POI src) as it does not make sense to add DiffOpt as a dep for POI.
This should be either:
1 - An extension here (POI)
2 - A separate package
3 - An extension at DiffOpt
Semantically, option 3 makes lots of sense. But this uses too much of POI internals. Option 2 has a similar issue, we will have to pin a POI version.
Currently, I like 1.
Still requires:
cc @matbesancon, @blegat