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

Should we generate AbstractArray rand_tangent for all AbstractArray subtypes? #198

Open
oxinabox opened this issue Mar 8, 2021 · 1 comment

Comments

@oxinabox
Copy link
Member

oxinabox commented Mar 8, 2021

See discussion at https://discourse.julialang.org/t/chainrulescore-rrule-for-custom-struct-does-the-pullback-need-to-support-composite-explicitly/56734

We can maybe do something with zero and similar like Zygote does for getindex
https://github.com/FluxML/Zygote.jl/blob/956cbcf3c572c0eb09c146189bb38b1b434634ff/src/lib/array.jl#L47-L49
I find thought when i was working on the getindex for ChainRules (JuliaDiff/ChainRules.jl#240) trying to work this out for general case is actually really hard, i gave up and restricteds to x::Array{<:Number}.

But maybe for rand_tangent we don't need to do so well, we just need to make something that is more likely to be what the user expects than Composite is.
It can still screw up, but as long as it screws up less, or with a clearer error.

@willtebbutt
Copy link
Member

willtebbutt commented Mar 8, 2021

I maintain that rand_tangent currently has the correct default behaviour. As I see it, there are two things that need to be done.

First thing

  1. signpost it better: "if you've got a custom array type that isn't a StridedArray and you don't want to treat like a struct, you'll need to teach rand_tangent what to do",
  2. make this really easy to do. Either a macro, or just a function, so that implementing rand_tangent for your AbstractArray is a one-liner.

Second thing

Implement this so that it's easier for rule-implementers to get this right / to convert to their preferred types when writing rules.

@mzgubic mzgubic transferred this issue from JuliaDiff/ChainRulesTestUtils.jl Apr 8, 2021
@mzgubic mzgubic transferred this issue from JuliaDiff/FiniteDifferences.jl Jul 27, 2021
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

No branches or pull requests

2 participants