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

remove rule for identity #64

Merged
merged 1 commit into from
Aug 12, 2021
Merged

Conversation

KristofferC
Copy link
Contributor

This one causes quite a lot of invalidations (SciML/DiffEqBase.jl#698 (comment)). It was added in #54 but it is unclear why it was added.

Is it needed for anything?

@ChrisRackauckas
Copy link
Member

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
Copy link
Contributor Author

KristofferC commented Aug 11, 2021

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
Copy link
Member

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
Copy link
Member

Nabla failures unrelated @oxinabox ?

@oxinabox
Copy link
Member

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

@oxinabox
Copy link
Member

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 ChrisRackauckas merged commit 8c586af into JuliaDiff:master Aug 12, 2021
@ChrisRackauckas
Copy link
Member

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

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.

3 participants