search

invenia / Nabla.jl

A operator overloading, tape-based, reverse-mode AD
Other
68 stars 5 forks source link

`using Nabla` breaks `getindex(::Array)` #211

Open Jollywatt opened 3 years ago

Jollywatt commented 3 years ago

For example:

julia> [42][]
42

julia> using Nabla

julia> [42][]
ERROR: StackOverflowError:
Stacktrace:
     [1] getindex(::Vector{Int64}; kwargs::Base.Iterators.Pairs{Union{}, Union{}, Tuple{}, NamedTuple{(), Tuple{}}})
       @ Nabla ~/.julia/packages/Nabla/pRNW3/src/sensitivities/chainrules.jl:176
     [2] getindex(::Vector{Int64})
       @ Nabla ~/.julia/packages/Nabla/pRNW3/src/sensitivities/chainrules.jl:173
     [3] rrule(::typeof(getindex), ::Vector{Int64})
       @ ChainRules ~/.julia/packages/ChainRules/ZTLLo/src/rulesets/Base/indexing.jl:9--- the last 3 lines are repeated 26660 more times ---

Tested on:

(@v1.6) pkg> st Nabla
      Status `~/.julia/environments/v1.6/Project.toml`
  [49c96f43] Nabla v0.13.2

julia> versioninfo()
Julia Version 1.6.1
oxinabox commented 3 years ago

People outside Invenia are using Nabla? I don't really recommend it for new projects, we are hoping to deprecate Nabla at some point, once we migrate the last internal projects off it.

What is going wrong is that Nabla is generating a overload based on the ChainRule for getindex. Now it has a check in-place to make sure it never generates a rule that doesn't have at least 1 argument of Node type. https://github.com/invenia/Nabla.jl/blob/f5adedb5632d0ec9b887709e0c0391286d386603/src/sensitivities/chainrules.jl#L188

But it gets tricked by the vararg. Because it generates:

getindex(::Array,  ::Node...)

Which looks fine because one argument is a Node. But it isn't since as a vararg, there might be zero of these actually present.

So somehow we need to introduce special handling for varargs to make sure at least one is present if we are requiring that for purposes of making sure we only generare methods were we have changes at least one type to Node.

Jollywatt commented 3 years ago

People outside Invenia are using Nabla?

I don't really recommend it for new projects, we are hoping to deprecate Nabla at some point, once we migrate the last internal projects off it.

BTW, I'm new to AD and didn't know which to chose from the plethora of Julia AD packages — but I get the impression that Zygote.jl is the best for high-level use.

oxinabox commented 3 years ago

Welcome. Yes, Zygote is a good place to start. I also comaintain Zygote, and it is what Invenia uses for most new projects