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

Reconsider giving zero as the derivative of one #725

Closed
Keno opened this issue Jul 1, 2023 · 2 comments · Fixed by #726
Closed

Reconsider giving zero as the derivative of one #725

Keno opened this issue Jul 1, 2023 · 2 comments · Fixed by #726
Assignees

Comments

@Keno
Copy link
Contributor

Keno commented Jul 1, 2023

I'd like to reconsider the discussion in #187. Because of this change, we have the following unfortunate situation:

julia> using Diffractor, ZeroBundle
julia> using Diffractor: ZeroBundle
julia> using ChainRules: ZeroTangent
julia> Base.return_types(Diffractor.∂☆{2}(), Tuple{
	typeof(ZeroBundle{2}(/)),
	Diffractor.TangentBundle{2, Float64, Diffractor.TaylorTangent{Tuple{Float64, ZeroTangent}}},
	Diffractor.TangentBundle{2, Float64, Diffractor.TaylorTangent{Tuple{ZeroTangent, ZeroTangent}}},
})[1]

Diffractor.TangentBundle{2, Float64, Diffractor.TaylorTangent{Tuple{Float64, Float64}}}

vs with that rule set of ZeroTangent:

Diffractor.TangentBundle{2, Float64, Diffractor.TaylorTangent{Tuple{Float64, ZeroTangent}}}

i.e. because of said rule, we can't concluded that the second order derivative remains zero at type-level. This is unfortunate, because it introduces significant additional code generation. I of course can see both arguments - in general ChainRules has a bit of a type-stability problem because of these types, but I don't think there's any good reason to not return the sentinel type just for these particular functions.

@oxinabox
Copy link
Member

oxinabox commented Jul 2, 2023

Yes. I am happy to see it switched back.
We should also switch it back for @scalar_rule that change was motivated by ForwardDiff2.jl but since that's dead, I no longer see such need for it.

It's related to JuliaLang/julia#38241 in general.
maps are the place we get bitten.

@oxinabox
Copy link
Member

oxinabox commented Jul 4, 2023

We should also switch it back for @scalar_rule that change was motivated by ForwardDiff2.jl but since that's dead, I no longer see such need for it.

Looks like that was already done, or never existed in first place

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 a pull request may close this issue.

2 participants