Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Extra restructure in Enzyme for reinterpreted array #707

Closed
ChrisRackauckas opened this issue Aug 12, 2022 · 1 comment · Fixed by #922
Closed

Extra restructure in Enzyme for reinterpreted array #707

ChrisRackauckas opened this issue Aug 12, 2022 · 1 comment · Fixed by #922
Labels

Comments

@ChrisRackauckas
Copy link
Member

ChrisRackauckas commented Aug 12, 2022

MWE:

using Zygote, SciMLSensitivity, OrdinaryDiffEq

solver = Rodas3() # fails because of view https://github.com/SciML/PreallocationTools.jl/pull/32

p = rand(3)

function dudt(u, p, t)
    u .* p
end

function loss(p, sensealg)
    prob = ODEProblem(dudt, [3.0, 2.0, 1.0], (0.0, 1.0), p)
    sol = solve(prob, solver, dt = 0.01, saveat = 0.1, sensealg = sensealg,
                abstol = 1e-5, reltol = 1e-5)
    sum(abs2, Array(sol))
end

dp1 = Zygote.gradient(p -> loss(p, QuadratureAdjoint(autojacvec = EnzymeVJP())), p)[1]

The commit a669d4f works around SciML/PreallocationTools.jl#32 since Enzyme can only have Enzyme.DuplicatedNoNeed(y, tmp1) both as the same type. This is then required since:

function get_tmp(dc::DiffCache, u::AbstractArray{T}) where {T <: ForwardDiff.Dual}
    nelem = div(sizeof(T), sizeof(eltype(dc.dual_du))) * length(dc.du)
    if nelem > length(dc.dual_du)
        enlargedualcache!(dc, nelem)
    end
    _restructure(dc.du, reinterpret(T, view(dc.dual_du, 1:nelem)))
end

the return for tmp1 is a ReinterpretedArray instead of an Array. This makes the Rosenbrock case have an unnecessary allocation as it should in theory be able to be kept as a ReinterpretedArray, but this fix requires the extra allocation that SciML/PreallocationTools.jl#32 would have otherwise eliminated.

@ChrisRackauckas
Copy link
Member Author

To fix this, I think either escape analysis has to delete the wrapper allocation or Enzyme has to allow non-same types.

@vchuravy @wsmoses could that be possible?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant