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

rrules for getindex seems to be ignored with Zygote #239

Closed
oschulz opened this issue Oct 21, 2020 · 6 comments
Closed

rrules for getindex seems to be ignored with Zygote #239

oschulz opened this issue Oct 21, 2020 · 6 comments

Comments

@oschulz
Copy link
Contributor

oschulz commented Oct 21, 2020

With

using ChainRulesCore, Zygote

struct Foo <: AbstractArray{Int,0} end

running

myfunc(x::Foo) = throw(ErrorException("This should not be called"))
ChainRulesCore.rrule(::typeof(myfunc), x::Foo) = 99, _ -> (NO_FIELDS, 42)
Zygote.gradient(myfunc, Foo())

yields (42,), as expected. myfunc is bypassed and never called. However, the same with Base.getindex

Base.getindex(x::Foo) = throw(ErrorException("This should not be called"))
ChainRulesCore.rrule(::typeof(Base.getindex), x::Foo) = 99, _ -> (NO_FIELDS, 42)
Zygote.gradient(Base.getindex, Foo())

throws

ERROR: This should not be called

so Zygote seems to ignore that rrule and call Base.getindex.

I'm reporting this here, though I'm not sure if it's a problem in ChainRulesCore or Zygote (or maybe I'm doing something wrong here?).

@oschulz
Copy link
Contributor Author

oschulz commented Oct 21, 2020

This only happens with

struct Foo <: AbstractArray{Int,0} end

When using

struct Foo end

Zygote does not call Base.getindex.

@oschulz
Copy link
Contributor Author

oschulz commented Oct 21, 2020

Also, using Foo <: AbstractArray{Int,0} but ZygoteRules.@adjoint instead of ChainRulesCore.rrule

using Zygote, ZygoteRules

struct Foo <: AbstractArray{Int,0} end

Base.getindex(x::Foo) = throw(ErrorException("This should not be called"))
ZygoteRules.@adjoint Base.getindex(x::Foo) = 99, _ -> (42,)
Zygote.gradient(Base.getindex, Foo())

works as expected.

@oschulz
Copy link
Contributor Author

oschulz commented Oct 21, 2020

Given how lightweight ChainRulesCore is, I guess it's must be a Zygote issue - I'll move it over there. Sorry!

@oschulz oschulz closed this as completed Oct 21, 2020
@oschulz
Copy link
Contributor Author

oschulz commented Oct 21, 2020

Moved to FluxML/Zygote.jl#811 .

@willtebbutt
Copy link
Member

This is more of a feature than a bug.

With the way that Zygote is currently implemented, any applicable Zygote.@adjoint rule will supercede ChainRulesCore.rrules, even if the typing of the rrule is more specific than that of the Zygote.@adjoint.

This is exactly what's happening here -- Zygote has a Zygote.@adjoint defined for Base.getindex, so overloading rrule won't over-ride that. I'm assuming that the current getindex implementation is very loosely typed, which is bad, so that should be brought up on Zygote.

@oschulz
Copy link
Contributor Author

oschulz commented Oct 21, 2020

Thanks, @willtebbutt!

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