Use similar in creation of DiffResults buffer

[torfjelde]

Using similar means that we get automatic compat with several other packages, e.g. ComponentArrays.jl (the demo from the docs):

julia> x = ComponentVector(μ = 0.0, σ = 1.0);

julia> ℓ = ADgradient(:ForwardDiff, problem)
ForwardDiff AD wrapper for NormalPosterior{Float64}(100, -0.06359079034463508, 1.241856096172367), w/ chunk size 2

julia> LogDensityProblems.logdensity_and_gradient(ADgradient(:ForwardDiff, problem), x)[2]
ComponentVector{Float64}(μ = -6.359079034463508, σ = 25.339988478902235)

julia> ForwardDiff.gradient(Base.Fix1(LogDensityProblems.logdensity, problem), x) 
ComponentVector{Float64}(μ = -6.359079034463508, σ = 25.339988478902235)

Ref: TuringLang/AdvancedHMC.jl#301

[devmotion]

Should this still take into account dimension? I'm not sure but I assume there was a reason why it was used instead of x?

Suggested change

	DiffResults.MutableDiffResult(zero(S), (similar(x, S), ))
	DiffResults.MutableDiffResult(zero(S), (similar(x, S, dimension(ℓ)), ))

[tpapp]

Currently all it uses from xis its element type, and it takes the dimension from the first argument.

I think it could be done differently, using the dimension from x, and then maybe ℓ would not need to be an argument at all.

[torfjelde]

using the dimension from x

So similar basically does this. And AFAIK this is only called from the logdensity_and_gradient method, meaning that x should always have the correct dimensions.

Using dimension from ℓ instead will sometimes result in loss of structure of the original type for x, e.g. ComponentArrays.

[tpapp]

I would recommend something like

function _diffresults_buffer(x)
    T = eltype(x)
    S = T <: Real ? float(Real) : Float64 # heuristic
    DiffResults.MutableDiffResult(zero(S), (similar(x, S), ))
end

with the caller modified accordingly.

[torfjelde]

This is what I've impleented, right? Or are my eyes deceiving me?

[devmotion]

I think @tpapp's point is that the first argument of the function should be dropped if we do not use it anymore.

[devmotion]

That's not affected by the change above, is it?

[tpapp]

I also don't understand why it would not be needed.

[torfjelde]

Nah, it was a note by me so we I could ask why it's there:) As in, can we just remove it?

[tpapp]

What would you use instead?

The purpose was to collect everything related to extraction in a single function. I am open to suggestions, but simply removing it would break the package (the function is used).

[torfjelde]

Ah, I thought the ::Real was the reason for it, and was thinking maybe this is now redundant because type-inference is better / DiffResults might have fixed whatever issue required the ::Real in the first place. If that's not the case and it's just for convenience, then I'll remove the comment:)

[tpapp]

I no longer recall the case for ::Real, so I guess you can remove it. We can always put it back if things break and then document why it is there.

Please also remove the comment, and then I am happy to merge.

[torfjelde]

Done:)

[torfjelde]

Aaah of course. I'll get that sorted.

…

On Fri, Nov 18, 2022, 8:55 PM David Widmann ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In src/DiffResults_helpers.jl <#95 (comment)> : > @@ -14,7 +14,7 @@ Allocate a DiffResults buffer for a gradient, taking the element type of `x` int function _diffresults_buffer(ℓ, x) T = eltype(x) S = T <: Real ? float(Real) : Float64 # heuristic - DiffResults.MutableDiffResult(zero(S), (Vector{S}(undef, dimension(ℓ)), )) + DiffResults.MutableDiffResult(zero(S), (similar(x, S), )) I think @tpapp <https://github.com/tpapp>'s point is that the first argument of the function should be dropped if we do not use it anymore. — Reply to this email directly, view it on GitHub <#95 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ACUPZZBP2DACRCX3OIRFX2DWI7ULTANCNFSM6AAAAAASDRYGCA> . You are receiving this because you authored the thread.Message ID: ***@***.***>

[tpapp]

LGTM, just remove ::Real and the added comment.

[tpapp]

I no longer recall the case for ::Real, so I guess you can remove it. We can always put it back if things break and then document why it is there.

Please also remove the comment, and then I am happy to merge.

[tpapp]

LGTM, thanks!