Output a Symmetric from the Hessians

[ChrisRackauckas]

No description provided.

[coveralls]

Coverage decreased (-12.9%) to 54.895% when pulling 6cd3a09 on ChrisRackauckas-patch-1 into b478a2a on master.

[tpapp]

I think this is preferable, but note that this relies on all callers being written in a generic way (accepting <:AbstractMatrix instead of Matrix). Which is how it should be, but you may want to test it in typical applications.

[ChrisRackauckas]

@pkofod is Optim.jl fine with this?

[pkofod]

I’m not sure. This will only cause problems if you use the returned or object, and I don’t think we do. I guess the safe thing to do is then just to also input a matrix wrapped in symmetric right ?

But remind me again, why are we doing this if the triangle is already being copied?

[tpapp]

But remind me again, why are we doing this if the triangle is already being copied?

Generally, there are operations on symmetric (::Array etc) matrices that should, but don't preserve symmetry (numerically), while for ::Symmetric they could do this automatically. Not many are supported as primitives at the moment though.

[pkofod]

Okay. I guess I just prefer H in H out in these types of functions and then it and then people can decorate their stuff with symmetric as they like. I mean if they specifically want the symmetry-preserving (but potentially different in other aspects ) algorithms, then they call symmetric on it themselves. But I’m not really religious on this. As long as the triangle is copied.

[tpapp]

then people can decorate their stuff with symmetric as they like

In general, this assumes an understanding of the mathematical properties of the algorithm that may change, etc. I think the function that produces a value and can ensure a particular property should be the one signaling it.

As long as the triangle is copied.

My understanding is that this does not happen (or meaningless to rely on) for Symmetric.

[pkofod]

My understanding is that this does not happen (or meaningless to rely on) for Symmetric.

It does for the input H here, and that was specifically what I was referring to. For a mutating interface where you specifically pass in H it should either very clearly be documented that it only operates on the upper triangle or it should fill out everything.

[tpapp]

@pkofod: I think you are right. I missed the context, specifically that this is already a mutating function. I was thinking more along the lines of

julia> f(x) = Diagonal(x) # hessian of sum(x.^3 ./ 6)
f (generic function with 1 method)

julia> f!(H, x) = H .= f(x)
f! (generic function with 1 method)

julia> H = randn(3, 3)
3×3 Array{Float64,2}:
  0.974444  0.0354839  -0.241943
  2.32276   1.94886     0.657737
 -1.08576   1.24153    -0.549359

julia> f!(H, ones(3))
3×3 Array{Float64,2}:
 1.0  0.0  0.0
 0.0  1.0  0.0
 0.0  0.0  1.0

where the API is f but someone else in f! is using it in a mutating way.

So as given, this PR may be incorrect since if the caller gives a H::Matrix, they should expect that to be filled out.

[codecov]

Codecov Report

Merging #68 into master will not change coverage.
The diff coverage is 100%.

@@           Coverage Diff           @@
##           master      #68   +/-   ##
=======================================
  Coverage   67.66%   67.66%           
=======================================
  Files           7        7           
  Lines         402      402           
=======================================
  Hits          272      272           
  Misses        130      130

Impacted Files	Coverage Δ
src/hessians.jl	97.87% <100%> (ø)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b478a2a...6cd3a09. Read the comment docs.

[pkofod]

👍