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 a ForwardDiff extension for dimensionless quantities #765

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

Ickaser
Copy link

@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
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
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
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.

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