search

JuliaDiff / DiffRules.jl

A simple shared suite of common derivative definitions
Other
75 stars 38 forks source link

remove rule for identity #64

Closed KristofferC closed 3 years ago

KristofferC commented 3 years ago

This one causes quite a lot of invalidations (https://github.com/SciML/DiffEqBase.jl/pull/698#issuecomment-896872594). It was added in https://github.com/JuliaDiff/DiffRules.jl/pull/54 but it is unclear why it was added.

Is it needed for anything?

ChrisRackauckas commented 3 years ago

We need it in the symbolic stack because identity seems to show up from random things like linear algebra, so I think ForwardDiff.jl needs it too. It would need a lot of downstream tests to remove IMO given how many weird places we've seen this crop up. I would be scared of deleting it, even though it does cause invalidations.

KristofferC commented 3 years ago

Ok, we should add some test then. I don't see how the default fallback for identity doesn't work for ForwardDiff or the symbolic stack. Do you have an example?

ChrisRackauckas commented 3 years ago

Oh I see what you're saying.

idenity(f::Dual) = Dual(identity(f.val), identity(f.val) * f.der) == Dual(identity(f.val), identity(f.val)) == identity(f::Any)

For some reason I was thinking the derivative of the identity is 1, so identity(Dual(2,2)) == Dual(2,1)... but no you're right. The standard identity dispatch is fine, so ForwardDiff shouldn't be defining a Dual method on this.

ChrisRackauckas commented 3 years ago

Nabla failures unrelated @oxinabox ?

oxinabox commented 3 years ago

Nabla failures unrelated. Latest version of Nabla shouldn't even use DiffRules anymore.

oxinabox commented 3 years ago

I agree this is safe to remove. It is technically breaking. But it has only been there a month, and Symbolics.jl is the only thing doing anything new in that period that I am aware of. So if it doesn't hurt Symbolics then it is all good.

ChrisRackauckas commented 3 years ago

If it hurts Symbolics then we'll find out quickly so it's good. Rip off the bandaid.