Skip to content

Fix nested ForwardDiff tag mismatch #3406

Open
AdityaPandeyCN wants to merge 1 commit intoSciML:masterfrom
AdityaPandeyCN:patch/forwardDiff
Open

Fix nested ForwardDiff tag mismatch #3406
AdityaPandeyCN wants to merge 1 commit intoSciML:masterfrom
AdityaPandeyCN:patch/forwardDiff

Conversation

@AdityaPandeyCN
Copy link
Copy Markdown

Checklist

  • Appropriate tests were added
  • Any code changes were done in a way that does not break public API
  • All documentation related to code changes were updated
  • The new code follows the
    contributor guidelines, in particular the SciML Style Guide and
    COLPRAC.
  • Any new documentation only uses public API

Additional context

Fix for SciML/OrdinaryDiffEq.jl#3381. When NonlinearSolve computes a Jacobian over an ODE solve, p enters as Dual{NLTag} and Rosenbrock's internal ForwardDiff seeds u as Dual{OrdEqTag, Dual{NLTag}, CS}. The user function f(du, u, p, t) multiplies across tag levels, producing the wrong outer tag and crashing on setindex!.

Signed-off-by: AdityaPandeyCN <adityapand3y666@gmail.com>
Comment on lines +213 to +218
function (ff::SciMLBase.UJacobianWrapper{true})(du1, uprev::AbstractArray{<:ForwardDiff.Dual})
p = ff.p
if p isa AbstractArray && eltype(p) <: ForwardDiff.Dual
DualU = eltype(uprev)
if !(eltype(p) <: DualU)
hasfield(typeof(ff.f), :f) && getfield(ff.f, :f) isa FunctionWrappersWrappers.FunctionWrappersWrapper && return ff.f(du1, uprev, p, ff.t)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need to do it like this, it should just be in the solve dispatch?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried it but the DiffEqBase solve path can't promote p because the nested dual type doesn't exist yet at that point , it's created later by Rosenbrock's JacobianConfig. Promoting early caused triple nesting.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's incorrect to just unwrap the functionwrapper though, that means the wrong call is getting compiled, so it needs to be handled at the source of the issue.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense. I'll move the fix to jacobian! in derivative_wrappers.jl , widen p once before DI.jacobian! runs, same approach as #3389. or you could reopen that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants