bug fixes in QP backend #90
Conversation
Codecov Report
@@ Coverage Diff @@
## master #90 +/- ##
==========================================
+ Coverage 81.50% 81.59% +0.09%
==========================================
Files 5 5
Lines 865 864 -1
==========================================
Hits 705 705
+ Misses 160 159 -1
Continue to review full report at Codecov.
|
|
I see a major feature requirement (thanks to cvxpylayer), i.e. ability to track gradients (if we can somehow pass a TrackedArray instead of an array in a JuMP model). This allows fine-grained control of differentiating a function, which can be a composition of variables and tracked problem data (such as a loss) directly. |
|
Normally we shouldn't need tracked arrays. This is the way it was done in
Tracker, the former AD system for Flux. It's more a mechanism for
propagating gradient information rather than a feature in itself.
…On Sun, Apr 25, 2021, 16:22 Akshay Sharma ***@***.***> wrote:
I see a major feature requirement (thanks to cvxpylayer), i.e. ability to
track gradients (if we can somehow pass a TrackedArray instead of an array
in a JuMP model). This allows fine-grained control of differentiating a
function, which can be a composition of variables and tracked problem data
(such as a loss) directly.
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#90 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AB2FDMT25EP7T2J47CLRF33TKQQRVANCNFSM43RKNBPA>
.
|
src/MOI_wrapper.jl
Outdated
| @@ -347,12 +347,12 @@ end | |||
|
|
|||
| function MOI.get(model::Optimizer, | |||
| ::ForwardIn{QuadraticObjective}, vi1::VI, vi2::VI) | |||
| tuple = ifelse(index(vi1) <= index(vi2), (vi1, vi2), (vi2, vi1)) | |||
| tuple = ifelse(vi1.value <= vi2.value, (vi1, vi2), (vi2, vi1)) | |||
There was a problem hiding this comment.
why changing this one?
There was a problem hiding this comment.
it was failing on my device, on Julia 1.0
There was a problem hiding this comment.
one thing to note: tuple is also a Julia function in Base, maybe try to use a name that is not already defined to avoid tricky bugs
| @@ -871,7 +871,7 @@ function _fill_quad_Q(model, dQv, dQi, dQj, index_map) | |||
| for ((vi1, vi2), val) in dict_dQ | |||
| i = index_map[vi1].value | |||
| j = index_map[vi2].value | |||
| for (vi, val) in dict | |||
| for (vi, val) in dict_dQ | |||
There was a problem hiding this comment.
ok this is weird that this wasn't caught by tests before
There was a problem hiding this comment.
@joaquimg should there be a test where dict_dQ gets used?
There was a problem hiding this comment.
yep. i guess with more examples common, such bugs (if any) will surface
|
@be-apt since this PR seems to be focused on the small bug fixes, maybe we should rename it and open something for the regression examples? |
akshay326
changed the title
|
is this good to go? |
|
rename the |
Wanted to emulate something like this example: https://github.com/cvxgrp/cvxpylayers/blob/master/examples/torch/tutorial.ipynb
Made some small fixes
Here's the demo: https://github.com/be-apt/jump-gsoc-2020/blob/master/sensitivity-analysis-ridge.ipynb