Skip to content

Add a ForwardDiff extension for dimensionless quantities #765

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

Merged
merged 5 commits into from
Jun 4, 2025

Conversation

Ickaser
Copy link
Contributor

@Ickaser Ickaser commented Jan 24, 2025

#682 seemed like low-hanging fruit, so this pull request solves that problem by adding a ForwardDiff extension with a method for convert very much like what the MethodError suggests. It is entirely possible that this method is too specific or not specific enough, but this fixes at least that MWE.

I would love to see this extension eventually allow other functionality, e.g. derivatives with respect to dimensional quantities, but this would be a start at least, and facilitates the case where units are only used inside a user-defined function.

@Ickaser
Copy link
Contributor Author

Ickaser commented Feb 14, 2025

@sostock or any other maintainers: is this reasonable? If it just needs some docs or tests, would be happy to contribute something; as it stands, this just fixes #682 .

Copy link
Collaborator

@sostock sostock left a comment

Choose a reason for hiding this comment

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

I have some small comments. A test should be added as well.

@Ickaser
Copy link
Contributor Author

Ickaser commented Feb 17, 2025

Should I care about avoiding a SciML solver package like OrdinaryDiffEqRosenbrock as a test dependency? I suspect adding that package would add a lot of compile time to testing, but it would also have the benefit of ensuring that this use case continues to function more broadly.

The basic problem here can be tested (and demonstrated to work) with a relatively simple test, it turns out:
ForwardDiff.Dual(1.0)*u"cm/m" + ForwardDiff.Dual(1.0) is the place where conversion breaks.
I've added that as a test now, so there is something at least now in the test suite making sure this works.

@Ickaser
Copy link
Contributor Author

Ickaser commented Mar 5, 2025

All of the failing checks are pre-1.9, so before package extensions existed, and they fail because the extension isn't loaded. I can do something like

if !isdefined(Base, :get_extension)
    include("../ext/ForwardDiffExt.jl")
end

as is done for the other two extensions, but this will error if ForwardDiff isn't installed. I'm not familiar enough yet with the details to know how easy it would be to make ForwardDiff an outright dependency for earlier versions of Julia, but that seems undesirable (this is the whole reason this is an extension).

I have seen conversations elsewhere about the possibility of dropping Julia 1.0 and 1.6 from CI (e.g. JuliaDiff/ForwardDiff.jl#731); for Unitful, it seems nice to support as far back as possible, but I don't know how that priority compares against new features like this pull request provides.

Edit: there's a manual section on this, here. So it would be relatively straightforward to make the ForwardDiff extension work for 1.9 and above, and make ForwardDiff an explicit dependency of Unitful on Julia <1.9, but then we have the question of whether solving #682 is worth adding a dependency for people using older versions of Julia.

@Ickaser
Copy link
Contributor Author

Ickaser commented May 15, 2025

Bump: question is whether to make ForwardDiff a dependency (rather than just an extension) for pre-Julia 1.9, in order to resolve #682 . My intuition is that not many people are using pre-LTS, and if so adding the ForwardDiff dep shouldn't be too painful (it's such a common package anyway).

@Ickaser
Copy link
Contributor Author

Ickaser commented May 26, 2025

Hi @sostock, as an attempt to make this a little easier to add, I decided to make the extension only tested for Julia 1.9 and up. That way CI is happy; the bug in #682 will persist for users with Julia 1.8 or lower, but be solved for everyone else.

Co-authored-by: Sebastian Stock <[email protected]>
@sostock sostock merged commit 2effe2e into JuliaPhysics:master Jun 4, 2025
16 checks passed
@sostock
Copy link
Collaborator

sostock commented Jun 4, 2025

Thank you for your contribution!

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.

2 participants