ForwardDiff.jl icon indicating copy to clipboard operation
ForwardDiff.jl copied to clipboard

Added Functionality for FieldVectors (from StaticArrays)

Open kylejbrown17 opened this issue 7 years ago • 3 comments

Hi Team,

This is my attempt to extend all SArray functionality to FieldVectors. This resolves issue #305. The basic approach mostly involves replacing SArray with Union{FieldVector, SArray} in method definitions. I have included tests to mirror those used for testing SArray codepaths for Gradients, Hessians and Jacobians.

kylejbrown17 avatar Feb 28 '18 23:02 kylejbrown17

@kylejbrown17 thanks! I added some more tests, and it looks like there is a stack overflow when calculating the jacobian. We should track this down and fix it before merging. Can you give me push access to your fork of this repo so that I can push to this PR? Thanks!

zsunberg avatar Mar 03 '18 21:03 zsunberg

Oh, ok, I see the problem... Not sure if it is by design. The issue is in StaticArrays.jl. Will file an issue there. I fixed the tests, so this should be ready to merge.

zsunberg avatar Mar 04 '18 05:03 zsunberg

I was interested in using FieldVectors in combination with ForwardDiff and asked in the Julia Slack channel for help since this PR has already been open for a long time. Mateusz Baran suggested the following:

ForwardDiff.dualize calls StaticArrays.similar_type at generation time, not call time, which means new methods of that function may or may not be picked (like the one you've added). This modification solves it:

@generated function ForwardDiff.dualize(::Type{T}, x::StaticArray) where T
    N = length(x)
    dx = Expr(:tuple, [:(ForwardDiff.Dual{T}(x[$i], chunk, Val{$i}())) for i in 1:N]...)
    return quote
        $(Expr(:meta, :inline))
        chunk = ForwardDiff.Chunk{$N}()
        V = StaticArrays.similar_type($x, $(ForwardDiff.Dual{T,eltype(x),N}))
        return V($(dx))
    end
end

I think this should be fine. I'd say my version is safer than the one currently present in ForwardDiff but both the current one and mine are a bit less safe then what was desired in that PR. I really doubt anyone would overload eltype or length for a subtype of FieldVector but that's actually where it's a bit unsafe.

To be honest these generated functions are a bit over my head but I wanted to share the suggestion from Mateusz Baran here to perhaps revive the discussion in this PR and to prevent the suggestion from getting lost due to Slack's message history limit.

oschub avatar Jan 29 '21 10:01 oschub