-
-
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
WIP: Fix getfield bug #852
Conversation
@@ -209,7 +209,7 @@ end | |||
|
|||
@generated pair(::Val{k}, v) where k = :($k = v,) | |||
|
|||
@adjoint function literal_getproperty(x, ::Val{f}) where f | |||
@adjoint function literal_getproperty(x, ::Val{f}, getproperty=getproperty) where f |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel this is going to lead to confusion since getproperty doesn't take a getproperty
argument.
I thikn better is to declare a literal_getfield
that matches literal_getproperty
for getfun in (:getproperty, :getfield)
@eval @adjoint function $(Symbol(:literal_, $getfun)(x, ::Val{f}) where f
val = $getfun(x,f)
...
end
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was added in FluxML/ZygoteRules.jl#13. My thinking was that it should be more backwards compatible this way, but maybe we should go with your proposal instead.
@@ -228,7 +228,7 @@ _pullback(cx::AContext, ::typeof(getproperty), x, f::Symbol) = | |||
_pullback(cx, literal_getproperty, x, Val(f)) | |||
|
|||
_pullback(cx::AContext, ::typeof(getfield), x, f::Symbol) = | |||
_pullback(cx, literal_getproperty, x, Val(f)) | |||
_pullback(cx, literal_getproperty, x, Val(f), getfield) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and to match my suggestion above
_pullback(cx, literal_getproperty, x, Val(f), getfield) | |
_pullback(cx, literal_getfield, x, Val(f)) |
Also note that forward mode has the same issues, so that should probably be fixed as well. |
Hey, thanks for reviewing. It looks like you've taken this into account in #848 now, am I correct? In which case we can close this? |
Yes, I think closing in favor of #848 makes sense, since other than fixing #851, #848 only really marks two tests as broken in 1.6, so I don't think splitting these changes up is worthwhile. (I also missed this before, but note that this PR on its own isn't enough to fix #851, since |
Closes #851
Will add tests