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

Add single-pass value_and_derivative(s) functions #678

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

gerlero
Copy link

@gerlero gerlero commented Dec 19, 2023

Closes #610.

Proposal

I propose adding a value_and_derivative function for easy single-pass computation of value and derivative that has the same signature as derivative but returns a plain tuple. I also propose a value_and_derivatives function that also includes the second derivative. I'm not proposing methods for even higher order derivatives (honestly because I don't have a need for those), but if needed these could be added in the future via an optional argument (e.g. aVal{n}) in value_and_derivatives.

Rationale

The rationale is the one I presented in #610. To mention it here, using the DiffResults API (which is intended for these use cases) for very simple scalar derivatives is somewhat convoluted, crucially requiring knowledge of the return types of the differentiated function ahead of time instead of just being able to let the compiler deal with the types.

This has caused implementations of functions like value_and_derivative to appear in other packages (including mine); but these have to rely on internals of ForwardDiff (i.e., Dual and related functions), which is not ideal.

Naming

I considered the names value_and_derivative and value_derivative. I decided on the former as that's the name that was chosen in the AbstractDifferentiation package.

Background

Initially I wanted to register a wrapper package with this functionality, but Registry maintainers asked me to consider a PR here instead. I agree and I hope these changes can be merged here.

@gerlero
Copy link
Author

gerlero commented Dec 19, 2023

CI failures seem to be unrelated to these changes

@devmotion
Copy link
Member

I decided on the former as that's the name that was chosen in the AbstractDifferentiation package.

I wonder if AbstractDifferentation.value_and_derivative should be implemented in this way (which might make this PR obsolete?).

@gerlero
Copy link
Author

gerlero commented Dec 19, 2023

I wonder if AbstractDifferentation.value_and_derivative should be implemented in this way (which might make this PR obsolete?).

If it's acceptable for that package to rely on these ForwardDiff internals, I don't mind making the change in there. Just, can we add a higher-order value_and_derivatives function to that package? AFAICT there's no way to get values and two derivatives in a single pass using only documented functions.

@gerlero
Copy link
Author

gerlero commented Dec 19, 2023

Just, can we add a higher-order value_and_derivatives function to that package? AFAICT there's no way to get values and two derivatives in a single pass using only documented functions.

Sorry, I failed to notice that AsbtractDifferentiation already defines an interface for multiple derivatives (the signature is value_and_derivative(ab::AD.AbstractBackend, f, xs::Number...), BTW can this be type-stable?). So, this part could be ignored as ideally it would only require changes in the backend.

My bad. I got it wrong: that signature is for multiple function arguments; not for higher-order derivatives.

There's a value_gradient_and_hessian function though, so a value_and_derivatives function would fit right in there.

@gerlero
Copy link
Author

gerlero commented Dec 20, 2023

I had a few minutes to play around with making these changes in AbstractDifferentiation instead (to complete the interface I also added second_derivative and value_and_second_derivative; which I guess I could add to this PR too).

While implementing the changes in AbstractDifferentiation is obviously viable, I have to say that that package's interface doesn't feel really comfortable when working with simple scalar functions (the nested tuple returns make debugging and getting it right in the first try more difficult). For this reason plus the already mentioned fact that the implementation relies on what are ForwardDiff internals, I'd really prefer that these functions be added here in ForwardDiff if at all possible.

@gerlero
Copy link
Author

gerlero commented Jan 2, 2024

For this reason plus the already mentioned fact that the implementation relies on what are ForwardDiff internals, I'd really prefer that these functions be added here in ForwardDiff if at all possible.

Gentle bump @devmotion; are you willing to consider adding these functions to this package? (regardless of whether equivalent functions go into AbstractDifferentiation too)

@devmotion
Copy link
Member

devmotion commented Jan 3, 2024

IMO there is no need for duplicate definitions, in particular given that the long-term goal of AbstractDifferentation is to move the extensions to the relevant packages (i.e., Zygote, Tracker, ForwardDiff etc.).

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.

Is this code a supported use? (single-pass value and derivative)
2 participants