Fixed L-BFGS, included new code for BlockArray #3
Conversation
Codecov Report
@@ Coverage Diff @@
## master #3 +/- ##
=========================================
- Coverage 92.2% 90.9% -1.31%
=========================================
Files 37 36 -1
Lines 1386 1352 -34
=========================================
- Hits 1278 1229 -49
- Misses 108 123 +15
Continue to review full report at Codecov.
|
|
|
||
| # Constructors | ||
| #default | ||
| function LBFGS(T::Type, dim::NTuple{N,Int}, M::Int) where {N} |
There was a problem hiding this comment.
I would prefer to keep this constructor for consistency, this is the standard constructor for all operators:
Operator(T::Type, dim::Tuple, args...)
src/linearoperators/LBFGS.jl
Outdated
|
|
||
| mutable struct LBFGS{R, T <: BlockArray, M} <: LinearOperator | ||
| currmem::Integer | ||
| curridx::Integer |
There was a problem hiding this comment.
having Abstract Types in constructors creates Type Instability.
src/linearoperators/LBFGS.jl
Outdated
| H::R | ||
| end | ||
|
|
||
| function LBFGS(x::T, M::Integer) where {R, T <: BlockArray{R}} |
There was a problem hiding this comment.
this created problems as R for example when typing LBFGS( (zeros(3),zeros(Complex{Float64})), 3).
While typeof( (zeros(3),zeros(Complex{Float64})) ) <: BlockArray{Float64} == true apparently it couldn't be determined that R was Float64.
src/linearoperators/LBFGS.jl
Outdated
| L.ys_M[L.curridx] = ys | ||
| blockcopy!(L.s_M[L.curridx], L.s) | ||
| blockcopy!(L.y_M[L.curridx], L.y) | ||
| yty = real(vecdot(L.y, L.y)) |
There was a problem hiding this comment.
test passed also with vecdot but I guess it is more safe to use our blockvecdot no?
|
definitively agree with that. changed that! |
|
Ok! Let me propose some additional changes though, let's wait before merging. |
|
sure! actually why not waiting until the new solvers are done? |
This PR is to fix L-BFGS: now it is implemented for both
ArrayandTupleobjects at once.Furthermore, the newly introduced
block.jldefines more rigorously whatBlockArrayobjects are (that is,ArrayorTupleobjects) and all the associated operations and convenient broadcasts. This will eventually serve as a replacement fordeep.jl.