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

Nested AD for getindex #944

Closed
wants to merge 6 commits into from
Closed

Nested AD for getindex #944

wants to merge 6 commits into from

Conversation

axsk
Copy link

@axsk axsk commented Apr 12, 2021

This incorporates parts of #77 and fixes #820 by implementing the adjoint for the adjoint, i.e. nested derivative, for getindex, thus facilitating higher order AD.

I cannot asses which parts of #77 that I left out are further necessary, but this works for getindex and other methods depending on it (e.g. sum).

Keno and others added 5 commits February 23, 2019 16:52
This adds a bunch of definitions to make nested AD work and adds tests
for second-order AD. Unfortunately by the time we get to third-order AD,
the types get so large that base decides to go on vacation while it thinks
about whether or not it might be willing to compile a function with a type
of such complexity. Additionally, Zygote introduces some unnecessary stacks,
which then prevent higher order AD. I plan to work on both of those issues,
but in the meantime, here are the changes to Zygote required to make this
work.
@DhairyaLGandhi
Copy link
Member

Thanks for looking into this. We might need to be a bit careful here to assess performance and memory issues. We should also have good amount of testing to ensure we can handle the use cases we care for

gradient(x) do x
sin(x[1])
end[1][1]
end[1][1] == -sin(1.0)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we put all these under a "Nested AD" testset?

@mcabbott mcabbott added the second order zygote over zygote, or otherwise label Jul 4, 2022
@ToucheSir ToucheSir mentioned this pull request Nov 10, 2022
1 task
@CarloLucibello
Copy link
Member

closing as stale

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
second order zygote over zygote, or otherwise
Projects
None yet
Development

Successfully merging this pull request may close these issues.

∇getindex mutates, causing issues with higher order AD over getindex.
5 participants