BackwardOut{...Objective} -> BackwardOutObjective #121
Conversation
blegat
force-pushed
the
bl/backwardoutobj
branch
2 times, most recently
from
1eb828d
to
e0bc18e
Compare
Codecov Report
@@ Coverage Diff @@
## master #121 +/- ##
==========================================
- Coverage 83.49% 81.20% -2.30%
==========================================
Files 6 6
Lines 727 782 +55
==========================================
+ Hits 607 635 +28
- Misses 120 147 +27
Continue to review full report at Codecov.
|
| @@ -296,10 +310,37 @@ function MOI.set(model::Optimizer, ::BackwardInVariablePrimal, vi::VI, val) | |||
| return | |||
| end | |||
|
|
|||
| function MOI.get(model::Optimizer, ::BackwardOut{LinearObjective}, vi::VI) | |||
| return _get_dc(model, vi) | |||
| function MOI.get(model::Optimizer, ::BackwardOutObjective) | |||
There was a problem hiding this comment.
You rarely want all the sensitivities.
If you have a model with many variables this will waste a lot of time while the user might just care about a single variable coefficient.
|
that's a good point yup, that was the motivation for the MOI attributes in the first place |
|
this introduces several elements that are not yet explained? What's the MOItoJuMP and why needed? |
This is adressed in the last commit.
Yes, I should add docstrings. It seems more consistent to return a JuMP.AbstractJuMPScalar than a MOI.AbstractScalarFunction if the getter is called on a JuMP model. The fact that it holds a JuMP model allows it to check that the variables indices given belong to the right model, handle conversion to JuMP functions and printing. |
|
|
||
| function MOI.get(model::JuMP.Model, attr::BackwardOutObjective) | ||
| func = MOI.get(JuMP.backend(model), attr) | ||
| return JuMP.jump_function(model, func) |
There was a problem hiding this comment.
do we still need the function below this one?
There was a problem hiding this comment.
Yes, we still have BackwardOut for the constraints.
|
In view of the bug #122 being unnoticed by the tests and that I wanted to be sure that we get it right, I thought we could have having integration tests checking for the scalar products between forward and backward diffs, which are unlikely to be correct if there are buts. |
blegat
force-pushed
the
bl/backwardoutobj
branch
from
c5f6870
to
c5193e2
Compare
|
Any objection to merge ? |
| end | ||
|
|
||
| """ | ||
| struct MatrixScalarQuadraticFunction{T, VT, MT} <: MOI.AbstractScalarFunction |
There was a problem hiding this comment.
do we really need the full struct in the docstring?
|
Go for it! |
Follow up of #119
See #114
Closes #122
Closes #123
Closes #125