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

WIP: Fix getfield bug #852

Closed
wants to merge 1 commit into from
Closed

WIP: Fix getfield bug #852

wants to merge 1 commit into from

Conversation

mzgubic
Copy link
Collaborator

@mzgubic mzgubic commented Dec 11, 2020

Closes #851

Will add tests

@@ -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
Copy link
Member

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

Copy link
Member

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)
Copy link
Member

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

Suggested change
_pullback(cx, literal_getproperty, x, Val(f), getfield)
_pullback(cx, literal_getfield, x, Val(f))

@simeonschaub
Copy link
Member

Was this adapted from #848? The question then is, how we want to proceed with releases. Should we release another 0.5.x with this fix, or do we want to just release 0.6 as soon as possible, in which case, we might as well just merge all of #848 (unless anyone has concerns about changes in there).

@simeonschaub
Copy link
Member

Also note that forward mode has the same issues, so that should probably be fixed as well.

@mzgubic
Copy link
Collaborator Author

mzgubic commented Dec 14, 2020

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?

@simeonschaub
Copy link
Member

simeonschaub commented Dec 14, 2020

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 src/compiler/reverse.jl would still need to be changed to actually emit literal_getfield) Thanks for this anyways, and sorry I didn't communicate my changes better in #848!

@mzgubic mzgubic closed this Dec 14, 2020
@mzgubic mzgubic deleted the mz/getfield branch December 14, 2020 11:10
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

Successfully merging this pull request may close these issues.

Forward pass of getfield acts like getproperty in _pullback
3 participants