Add support for Jacobian-vector products#168
Conversation
yes, I definitely agree. I cannot remember why it is that way. In NLSolvers.jl I have hv in the scalar objective https://github.com/JuliaNLSolvers/NLSolvers.jl/blob/e1afd319c26086b348283d5225c8eb5ea8151612/src/objectives.jl#L12 |
we could even add hv to the TwiceDiff interface immediately if we wanted, keep the other one around for legacy reasons (or don't since all of this will be breaking anyway) |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #168 +/- ##
==========================================
- Coverage 93.72% 88.41% -5.31%
==========================================
Files 10 9 -1
Lines 701 872 +171
==========================================
+ Hits 657 771 +114
- Misses 44 101 +57 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@pkofod it took me quite some time to fix the constructors of Existing tests seem to pass now. I'll now add tests for the new functionality. |
| # Aliases with `j` (Jacobian) | ||
| # Do not apply to functions that involve Hessians! | ||
| # Maybe we should clearly separate functions that take gradients (=> scalar-valued functions) and Jacobians (=> array-valued functions)? | ||
| const only_fj! = only_fg! | ||
| const only_fj_and_jv! = only_fg_and_gv! | ||
| const only_fj_and_fjv! = only_fg_and_fgv! | ||
|
|
||
| const only_fj = only_fg | ||
| const only_fj_and_jv = only_fg_and_gv | ||
| const only_fj_and_fjv = only_fg_and_fgv | ||
| const only_j_and_fj = only_g_and_fg |
There was a problem hiding this comment.
I'm not sure about names and about this duplication in general. This would be somewhat consistent with the current names (I think).
|
@devmotion did you write any tests that you didn't push here? |
|
No, because I wanted to be sure about the desired design first. Which type of functions do we want to add? How do we want to name them? Do we want to add all kind of new constructors to |
|
Method error but I think JET points in the direction of the issue in the rewrite of TwiceDifferentiable |
devmotion
force-pushed
the
dmw/jvp
branch
3 times, most recently
from
2ea86c9 to
354fb0e
Compare
|
I see you added tests for hv, then only tests for jv remains I suppose? |
|
No, I just fixed those that had to be changed since the ...HV struct is removed. There's no tests yet since the API hasn't stabilized yet. Downstream tests in Optim don't pass yet, which I think we have to ensure first. All recent changes are due to requirements and broken tests (eg broken TwiceDifferentiable and TwiceDifferentiableHV) in Optim, so until everything works together I think it's likely that we'll have to go back and change something here. |
devmotion
force-pushed
the
dmw/jvp
branch
8 times, most recently
from
429c2b2 to
9794fbb
Compare
devmotion
force-pushed
the
dmw/jvp
branch
2 times, most recently
from
24a372d to
13a2a6f
Compare
|
@devmotion what's the mentioned inconsistency with finite differencing that you refer to in the code comments? |
|
bump on this one, as it's the first in the sequence :) @devmotion |
|
Do you mean
? It refers to the observation I made when I enabled AD by using the same AD backend for all functions: Tests with finite differencing started to fail, since the finite differencing JVP is not identical to the finite differencing Jacobian multiplied with the desired vector. |
2 participants
A first incomplete sketch (the
make_...functions and tests are missing).I tried to avoid creating a new
...Differentiabletype, I think it's a bit unfortunate that there exists bothTwiceDifferentiableandTwiceDifferentiableHV.