-
-
Notifications
You must be signed in to change notification settings - Fork 213
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
map
overreach?
#646
Comments
Sounds fine to me. This is kind of a tricky tradeoff unfortunately; probably the only real answer is to delete the adjoint entirely and support differentiating through map, but that won't work right now. |
I kind of agree. There's definitely something to be exploited in knowing that I'll try to remember to make a PR on this soon. |
Perhaps we could kill two birds with one stone if JuliaDiff/ChainRules.jl#314 gets implemented. Moving to ChainRules would also get us ProjectTo, which in theory could handle more array types (modulo efficiency concerns). |
Zygote's current map implementation is arguably a bit optimistic about the types of things that it's able to handle.
For example, this issue in KernelFunctions.jl cropped up because we define a custom
AbstractVector
type that wraps aMatrix
, and lets it masquerade as a vector-of-vectors.Under the hood, this type makes sure to implement various operations efficiently on the wrapped matrix. It would be reasonable to assume that Zygote would be able to exploit these efficient implementations (because composition), but instead it hits the
map
adjoint and literally treats the object as a vector-of-vectors, which is bad for performance.I would propose to impose further type constraints on the implementation of
map
, perhaps toStridedArray
orDenseArray
, whichever is deemed a better target. @MikeInnes @dhairyagandhi96 any thoughts?The text was updated successfully, but these errors were encountered: