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

Support derivatives of functions with units via Unitful extension #680

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

Conversation

gerlero
Copy link

@gerlero gerlero commented Dec 21, 2023

Adds support for taking derivatives of functions with Unitful units on their input and/or output by means of an extension (weakdep). The extension works by ensuring that Dual always wraps a Real number—as ForwardDiff expects—even when Unitful.Quantitys are involved, stripping and adding back the units as appropriate.

Relevant issue is #328.

With this PR only derivative has support for units, yet the same behavior could be later applied to the other functions as well.

@gdalle
Copy link
Member

gdalle commented Dec 21, 2023

Thank you for the contribution! Can I ask why you would prefer this code to be in ForwardDiff?
My justification for submitting a Unitful extension is that ForwardDiff is the more "generic" / "mainstream" package, and so Unitful should strive to adapt to it rather than the other way around. Does that make sense?

@gerlero
Copy link
Author

gerlero commented Dec 21, 2023

@gdalle Sure enough. While the same code would also work as an extension for Unitful, my reasoning for submitting it here is that it is using ForwardDiff internals (i.e., the Dual API, which is not documented).

Alternative implementations of the extension that only extend the public API (e.g., adding a new method to derivative) that I could come up with can't cover all bases, as a derived function with units on its output (which the extension needs to strip from the Dual return before ForwardDiff is able to process it) may accept dimensionless (Real) input, and therefore we can't rely on dispatch for that.

EDIT: fix typo

@gerlero
Copy link
Author

gerlero commented Jan 2, 2024

Soft bump to the maintainers; interested in knowing if you're able to take these changes.

@gdalle
Copy link
Member

gdalle commented Jan 3, 2024

While I am a member of the JuliaDiff organization, I have never really maintained this repo so I probably won't be the one to merge this.
In my mind conceptually this belongs in Unitful. I agree that the names value, derivative, extract_derivative, Dual and Tag are not documented, but I'm willing to bet many packages actually rely on those internals too. So maybe the right question to ask would be #682

@jariji
Copy link

jariji commented Mar 29, 2024

This has unexpected results for some functions like sin for DimensionfulAngles.jl. Is it making certain assumptions about the derivative?

For reference I am looking at "On the dimension of angles and their units" which provides a good meaning of sin for unitful arguments.

Demonstration follows.

Just loosening the derivative bound from Real to Number and defining can_dual:

using ForwardDiff, Unitful, DimensionfulAngles

# Define a cycle unit equal to 360 degrees.
@unit cyc "cyc" Cycle 360u"°ᵃ" false


ForwardDiff.can_dual(::Type{Quantity{Float64}}) = true
function deriv(f::F, x::R) where {F,R<:Number}
    T = typeof(ForwardDiff.Tag(f, R))
    return ForwardDiff.extract_derivative(T, f(ForwardDiff.Dual{T}(x, one(x))))
end

julia> deriv(sin, 2π)
1.0

julia> deriv(sin, 1.0cyc)
1.0

julia> deriv(sin, 2π*u"rad")
1.0

julia> deriv(sin, 360.0*u"°ᵃ")
1.0

Versus this PR:

julia> ForwardDiff.derivative(sin, 0)
1.0

julia> ForwardDiff.derivative(sin, 1.0cyc)
6.283185307179586 cyc^-1

julia> ForwardDiff.derivative(sin, 2π*u"rad")
1.0 rad^-1

julia> ForwardDiff.derivative(sin, 360.0*u"°ᵃ")
0.017453292519943295 °^-1

@gerlero
Copy link
Author

gerlero commented Apr 8, 2024

@jariji My point with this PR was to support Unitful (which is what I use). I'm not familiar with DimensionfulAngles, but if small changes can be made here to support it then it should be done; otherwise I guess that can be left to a different PR.

@jariji
Copy link

jariji commented Apr 8, 2024

DimensionalAngles.jl just defines some more dimensions and units for Unitful.jl and their behavior in Base functions.

It's possible I'm misunderstanding the correct behavior -- I'm not an expert in quantities or autodiff. However, imho a function that gives right answers for some units and wrong answers for some units is probably not safe to merge because that kind of decision is the source of Julia's many correctness problems.

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