Remove frule for getindex(::Tuple, i) #680
Conversation
github-actions
bot
added
the
needs version bump
|
Probably makes sense to actually add an frule for |
There was a problem hiding this comment.
I will take your word for it, but right now this branch leads to the following failure:
julia> Diffractor.PrimeDerivativeFwd(x -> (x,2x,3x)[2])(1.0)
2.0
julia> Diffractor.PrimeDerivativeFwd(x -> sum((x,2x,3x)[1:2]))(1.0)
ERROR: type CompositeBundle has no field partials
Stacktrace:
[1] getproperty
@ ./Base.jl:37 [inlined]
[2] (::Diffractor.∂☆{1})(f::Diffractor.UniformBundle{1, typeof(getfield), ZeroTangent}, x::Diffractor.CompositeBundle{1, Tuple{Float64, Float64, Float64}, Tuple{Diffractor.TangentBundle{1, Float64, Tuple{Float64}}, Diffractor.TangentBundle{1, Float64, Tuple{Float64}}, Diffractor.TangentBundle{1, Float64, Tuple{Float64}}}}, s::Diffractor.TangentBundle{1, Int64, Tuple{NoTangent}}, inbounds::Diffractor.UniformBundle{1, Bool, ZeroTangent})
@ Diffractor ~/.julia/dev/Diffractor/src/stage1/forward.jl:165
|
I need to update Diffractor. We can hold this branch until I can push that. |
|
Can we rebase and merge this? |
Keno
force-pushed
the
kf/nofruletupgetindex
branch
from
8507e03 to
d169a50
Compare
|
Rebased and also gave the same treatment to |
|
CI is failing,
This hasn't been done yet? |
Having this chain rule is sub-optimal, because it prevents early-SROA in Diffractor-like systems that would like to perform some optimizations before applying AD (but can't do any optimization on functions that have custom rules). By letting it go down to the `getfield`, regular SROA can apply. Any AD system should handle `getfield` anyway, so I don't think there's a strong reason to have this. Similar reasoning applies to the reverse rules also, but they aren't currently actively causing me problems, so this PR only removes the frule, since I don't think many other packages are using them. We can revisit the rrules later.
For similar reasons as getindex, having a rule for first/tail is suboptimal because it supresses early SROA. Tail is particularly problematic, because it is used in the implementation of the ``` x, y... = abc ``` syntax, of which users expect early elimination.
Keno
force-pushed
the
kf/nofruletupgetindex
branch
from
d169a50 to
f5cab7d
Compare
|
I've deleted the unnecessary tests and added the getfield frule. Let's see if this works. |
|
All CI failures were preexisting |
Having this chain rule is sub-optimal, because it prevents early-SROA in Diffractor-like systems that would like to perform some optimizations before applying AD (but can't do any optimization on functions that have custom rules). By letting it go down to the
getfield, regular SROA can apply. Any AD system should handlegetfieldanyway, so I don't think there's a strong reason to have this.Similar reasoning applies to the reverse rules also, but they aren't currently actively causing me problems, so this PR only removes the frule, since I don't think many other packages are using them. We can revisit the rrules later.