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

Switch from AbstractDifferentiation to DifferentiationInterface #93

Merged
merged 5 commits into from
Oct 1, 2024

Conversation

gdalle
Copy link
Contributor

@gdalle gdalle commented Jul 17, 2024

Following our discussion per email, this PR proposes a switch from AbstractDifferentiation.jl to DifferentiationInterface.jl, which is becoming the new standard in the ecosystem.

  • Modify Project.toml files and imports
  • Replace SomethingBackend() with AutoSomething()
  • Replace value_and_gradient_closure with value_and_gradient (unclear how performance is affected)
  • Update documentation and README
  • Add preparation mechanism: available on another branch but not sure we want it because if the function contains value-dependent control flow, preparation is not appropriate

@gdalle
Copy link
Contributor Author

gdalle commented Jul 18, 2024

Any idea why the benchmarks fail? Or opinions on the preparation mechanism, which I could add to the present PR?

@lostella
Copy link
Member

@gdalle thanks for this! I need to take a deeper look into why the benchmarks fail, seems like something ends up in an endless loop, so maybe it's something to fix upstream.

Regarding preparation: maybe it can be added separately, with some mechanism to make it optional

@lostella
Copy link
Member

@gdalle #94 fixed the benchmark, apologies for the conflicts

@gdalle
Copy link
Contributor Author

gdalle commented Jul 22, 2024

Fixed the conflicts, this should be good to go! We can always discuss preparation in a separate PR

@gdalle
Copy link
Contributor Author

gdalle commented Jul 24, 2024

@lostella can you authorize the workflows?

@gdalle
Copy link
Contributor Author

gdalle commented Jul 24, 2024

It seems the benchmarks worked for the PR branch but failed for the main branch?

@lostella
Copy link
Member

Yes I think this is expected, since the change is breaking and the benchmark code won't work with the previous commit, right? I don't know of a better method to organize benchmarks for these cases.

I think it's fine, nothing to worry about. I'll just take a manual look at performance before merging, otherwise this looks good, thank you!

Project.toml Outdated Show resolved Hide resolved
Co-authored-by: Lorenzo Stella <[email protected]>
@gdalle
Copy link
Contributor Author

gdalle commented Jul 29, 2024

Hi @lostella, just popping by to see if you need anything else from me

@lostella lostella merged commit 27f8a96 into JuliaFirstOrder:master Oct 1, 2024
7 of 8 checks passed
@gdalle gdalle deleted the gd/differentiationinterface branch October 1, 2024 09:28
@gdalle
Copy link
Contributor Author

gdalle commented Oct 1, 2024

While we're at it, do you want to try to update to DI v0.6, which just landed?

@lostella
Copy link
Member

lostella commented Oct 1, 2024

While we're at it, do you want to try to update to DI v0.6, which just landed?

Makes sense!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants