Disable thunks for 2nd order AD #683
Conversation
src/tangent_types/thunks.jl
Outdated
| return quote | ||
| $(esc(_usethunks))() ? | ||
| Thunk($(esc(func))) : | ||
| $(esc(func))() | ||
| end |
There was a problem hiding this comment.
Comments on this approach over :(_usethunks() ? Thunk($(esc(func))) : $(esc(body))), from #568? My hope there was that including the body directly, instead of making and calling a function, might be slightly simpler for e.g. Zygote to understand.
| # Basically `:(Thunk(() -> $(esc(body))))` but use the location where it is defined. | ||
| # so we get useful stack traces if it errors. | ||
| func = Expr(:->, Expr(:tuple), Expr(:block, __source__, body)) |
There was a problem hiding this comment.
This could go further and generate a name for the thunk, based on source location, instead of anon. () ->. I think the name will be seen sometimes where the code line info is not?
But not this PR perhaps...
|
This was not a patch change. This is a new feature. I recommended being careful about this and biasing towards doing minor releases because it makes it possible to do backports in between releases Not a huge deal as we are unlikely to need to put in patches. |
|
Fair point. Initially this PR added a hook that users could register that would allow to disable thunking and so the original behavior was not changed, hence the patch version. But then I changed the PR to disable thunking for 2nd order AD unconditionally and forgot to change the version bump from patch to minor. |
|
That would still have been a new feature and would have been a minor. |
Needed for lazy Zygote:
FluxML/Zygote.jl#966
Also bumped patch version so that we can tag a release afterwards.