Add option to store the previous Jacobian in ShiftedComposite#150
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #150 +/- ##
==========================================
+ Coverage 73.71% 77.39% +3.67%
==========================================
Files 22 24 +2
Lines 898 969 +71
==========================================
+ Hits 662 750 +88
+ Misses 236 219 -17 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
MaxenceGollier
force-pushed
the
update_shiftedcomposite
branch
from
6684609 to
8cd8013
Compare
dpo
left a comment
dpo
left a comment
There was a problem hiding this comment.
Just a quick comment for now. I didn't look at the details, but the unit tests are failing.
| ) | ||
| end | ||
|
|
||
| A_prev = store_previous_jacobian ? copy(A) : nothing |
There was a problem hiding this comment.
Isn't there going to be a new allocation on each copy here? What kind of type is M? Maybe this is where copyto!() is useful?
There was a problem hiding this comment.
M is a SparseMatrixCOO, i added a commit to make copies on shift!, I am not sure whether copyto!() is really necessary.
copy is fine in the constructor.
There was a problem hiding this comment.
copy makes a copy, i.e., allocates a new object.
There was a problem hiding this comment.
When we construct the object we expect to allocate a copy of A i don't see where else we should allocate it than in the constructor ?!
|
They are indeed failing but I am not sure why, it looks like they are failing on unrelated code. Moreover, it is weird to check for |
|
Something allocated before no longer does (probably because of updates to julia itself). Something that didn't allocate before now allocates. So the tests will have to be adjusted. |
Done in #151 |
|
@dpo, can you review when you have some time ? I think this is ready. |
|
@dpo, Thank you and happy new year. |
MaxenceGollier
merged commit 268af6f
into
JuliaSmoothOptimizers:master
2 participants
@dpo,
This is useful when i make quasi-Newton updates for the Hessian of the Lagrangian (eq 18.13 in Nocedal & Wright).
Additionnally, I add full row rankness of the Jacobian as a struct field.
I used this code a few months ago for the different benchmarks in the SIOPT paper.
I will need a bump to a new version afterwards.