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.

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