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

Give Diffractor a ForwardDiff inspired interface #158

Closed
wants to merge 2 commits into from

Conversation

oscardssmith
Copy link
Member

@oscardssmith oscardssmith commented May 30, 2023

This is still somewhat WIP, but the goal is to make an exported interface for users to use. I'm currently running into some issues since this changes gradient to be forward mode (since forward mode is generally a lot more reliable currently).

@oscardssmith oscardssmith requested review from oxinabox and Keno May 30, 2023 20:32
oscardssmith added a commit to oscardssmith/Diffractor.jl that referenced this pull request May 31, 2023
oscardssmith added a commit that referenced this pull request May 31, 2023
@oxinabox
Copy link
Member

oxinabox commented Jun 1, 2023

I think the better way to do this is to extend AbstractDifferentiation.jl
which exports names like this.

I mean this is a partway step towards that, so we could do this first then that, but idk the motivation goes down once the short term need is met.
And we would like to have it working with AbstractDifferentiation eventually anyway.

This does mean doing derivative(DiffractorFwdBackend(), f, x) rather than Diffractor.derivative(f, x) but that seems fine.
And it gives us trivial swap-out for other ADs.
And gives us other things like lazy jacobian matrix-like operators (ForwardDiff2 style)

@staticfloat
Copy link
Collaborator

This does mean doing derivative(DiffractorFwdBackend(), f, x) rather than Diffractor.derivative(f, x) but that seems fine.

So can we just define derivative(::DiffractorFwdBackend, f, x) = Diffractor.derivative(f, x) and call it a day?

@oxinabox
Copy link
Member

oxinabox commented Jul 13, 2023

We should indeed be able to do something like that yes.

Basically we need to define one of these 4 functions
https://github.com/JuliaDiff/AbstractDifferentiation.jl/blob/master/src/AbstractDifferentiation.jl#L495-L502
Then everything else is taken care of for free.

For example https://github.com/JuliaDiff/AbstractDifferentiation.jl/blob/041d760d4c0bff8a5e023f9d1d5cad2ffbb68f4d/ext/AbstractDifferentiationForwardDiffExt.jl#L28

One can define more to include optimized versions of stuff like jacobian (which we will want to do at some stage.). But it is not required.


```julia
using Diffractor: var"'"
```
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 not do this as part of this PR?

@oxinabox oxinabox mentioned this pull request Jul 14, 2023
@oscardssmith
Copy link
Member Author

Closing in favor of #187

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.

3 participants