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

Replace AD package extensions with DifferentiationInterface #51

Merged
merged 10 commits into from
Nov 27, 2024

Conversation

gdalle
Copy link
Contributor

@gdalle gdalle commented Nov 23, 2024

Fixes #49. Here are the main changes:

  • Add dependency on ADTypes and DifferentiationInterface
  • Implement _derivative, _gradient, _jacobian and _hessian using DI
  • Remove package extensions for FiniteDiff, FiniteDifferences, ForwardDiff, ReverseDiff, Zygote
  • Replace backend constructors with their ADTypes counterparts everywhere.
  • Remove package extensions from docs.
  • Figure out what to do about AbstractDiffBackend, since it is apparently used downstream. Replace it with Union{ADTypes.AbstractADType, ExplicitEmbeddedBackend}? Right now I removed annotations in this package, and set AbstractDiffBackend = Any for downstream uses.
  • Fix broken test. It seems that we try to compute a Jacobian for a function with a scalar output?
  • Bump version to v0.4.0.
  • Undo changes for testing with next Manifolds version.

@mateuszbaran
Copy link
Member

Thanks for working on this update 👍

  • Figure out what to do about AbstractDiffBackend, since it is apparently used downstream. Replace it with Union{ADTypes.AbstractADType, ExplicitEmbeddedBackend}? Right now I removed annotations in this package, and set AbstractDiffBackend = Any for downstream uses.

Since this is a breaking change, I think we can just remove AbstractDiffBackend entirely and switch to ADTypes.AbstractADType in downstream packages.

  • Fix broken test. It seems that we try to compute a Jacobian for a function with a scalar output?

It's technically valid but I don't think it's a feature we rely on anywhere. I think we can switch the test to use a vector-valued function.

@gdalle
Copy link
Contributor Author

gdalle commented Nov 24, 2024

Since this is a breaking change, I think we can just remove AbstractDiffBackend entirely and switch to ADTypes.AbstractADType in downstream packages.

I agree, but this would prevent us from testing here because the test dependencies contain a version of Manifolds.jl which still requires AbstractDiffBackend.

@mateuszbaran
Copy link
Member

Right, in this case making AbstractDiffBackend an alias for Any is useful.

@gdalle
Copy link
Contributor Author

gdalle commented Nov 24, 2024

The fact that we use Manifolds in testing also means we cannot bump ManifoldsDiff to v0.4.0 in the current state, because it is not permitted by the Manifolds compat bound. The right way would be testing with a dev version of Manifolds where the compat v0.4.0 has been added for ManifoldsDiff. That would also make the AbstractDiffBackend = Any hotfix superfluous.

Copy link

codecov bot commented Nov 24, 2024

Codecov Report

Attention: Patch coverage is 95.00000% with 1 line in your changes missing coverage. Please review.

Project coverage is 95.69%. Comparing base (b8c3464) to head (7a50d38).
Report is 9 commits behind head on main.

Files with missing lines Patch % Lines
src/ManifoldDiff.jl 83.33% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main      #51      +/-   ##
==========================================
+ Coverage   95.46%   95.69%   +0.22%     
==========================================
  Files          21       13       -8     
  Lines         375      395      +20     
==========================================
+ Hits          358      378      +20     
  Misses         17       17              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.


🚨 Try these New Features:

@mateuszbaran
Copy link
Member

That's what I sometimes do locally when making breaking changes but I don't know about any way to do it on CI.

@gdalle
Copy link
Contributor Author

gdalle commented Nov 24, 2024

The last commit gives you an example of how to do that, using my custom branch of Manifolds where the compat entry has been upgraded and the AbstractDiffBackend import has been removed.

Warning

Don't merge before undoing those changes.

@mateuszbaran
Copy link
Member

Nice, I didn't know that trick.

@kellertuer
Copy link
Member

This indeed looks like a very neat trick! Thanks for illustrating that here.

@gdalle
Copy link
Contributor Author

gdalle commented Nov 24, 2024

Before we go any further here, I'm making sure that Manifolds itself can handle this ManifoldsDiff v0.4.0. I have opened a symmetric PR (JuliaManifolds/Manifolds.jl#771) which checks out my custom branck of ManifoldsDiff.

@gdalle
Copy link
Contributor Author

gdalle commented Nov 27, 2024

All the tests are passing with my custom branch of the downstream Manifolds (except codecov). I will now remove my Pkg tricks, the tests will thus fail but it will still be good to merge.

@gdalle
Copy link
Contributor Author

gdalle commented Nov 27, 2024

@kellertuer this is good to go

Copy link
Member

@mateuszbaran mateuszbaran left a comment

Choose a reason for hiding this comment

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

LGTM.

By the way, is JuliaManifolds/Manifolds.jl#772 good to go also?

@gdalle
Copy link
Contributor Author

gdalle commented Nov 27, 2024

Yeah you can ship that one too

@mateuszbaran mateuszbaran marked this pull request as ready for review November 27, 2024 13:44
@mateuszbaran mateuszbaran merged commit 1715f95 into JuliaManifolds:main Nov 27, 2024
4 of 13 checks passed
@gdalle gdalle deleted the gd/di branch November 27, 2024 13:45
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.

Using DifferentiationInterface for AD
3 participants