Skip to content

add ForwardDiff extension #178

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

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

Conversation

longemen3000
Copy link
Contributor

@longemen3000 longemen3000 commented May 26, 2025

Ports the code from https://github.com/longemen3000/ForwardDiffOverMeasurements.jl into an extension.

This PR only takes care of ambiguous operations (like +(x::Measurement,y::Dual)). It does not require any change in hierarchy (Measurements.Measurement isa AbstractFloat and ForwardDiff.Dual isa Real).

What is missing is some methods defined exclusively for Measurements ((like Measurements.value and Measurements.measurement), as well as constructor functions, but that discussion and PR can come later. I added the definitions for value/uncertainty (just map those over the partials), and measurement (the derivative respect to the value is dx * 1 ± 0, the derivative respect to the uncertainty is dx * 0 ± 1)

What this PR allows is allowing code that uses AD via ForwardDiff to use Measurements as primal type.

related: longemen3000/ForwardDiffOverMeasurements.jl#3

Adds a solution to #154

Copy link

codecov bot commented May 26, 2025

Codecov Report

Attention: Patch coverage is 96.87500% with 3 lines in your changes missing coverage. Please review.

Project coverage is 95.69%. Comparing base (ece3021) to head (2a41c0c).

Files with missing lines Patch % Lines
ext/MeasurementsForwardDiffExt.jl 96.84% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #178      +/-   ##
==========================================
+ Coverage   95.54%   95.69%   +0.14%     
==========================================
  Files          15       16       +1     
  Lines         764      860      +96     
==========================================
+ Hits          730      823      +93     
- Misses         34       37       +3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@longemen3000
Copy link
Contributor Author

errors in nightly seem unrelated

@giordano
Copy link
Member

giordano commented May 28, 2025

Any chance of either increasing code coverage or removing the untested functions? I'm not particularly familiar with ForwardDiff internals, there's lots of code I have no idea what's doing and seeing it's not even tested makes me a bit uncomfortable 🙂

@longemen3000
Copy link
Contributor Author

I can try. I'm gonna add some comments too. if you want some peace of mind, the code in this PR was already used some time ago in the making of this paper

@longemen3000
Copy link
Contributor Author

Some updates:

While testing for all possible combinations of numbers, Duals and measurements, i found #179. This PR adds a fix to that issue because it is necessary for the ForwardDiff tests. Also added some explanations about what does each code section do.


This causes ForwardDiff to fail, as it expects that typeof(one(T)) == typeof(zero(T)) == T.

The error is in ForwardDiff, as the correct function to use is oneunit(T).
Copy link
Member

@giordano giordano May 31, 2025

Choose a reason for hiding this comment

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

To be clear, if JuliaDiff/ForwardDiff.jl#665 was merged we wouldn't need all these methods? It really sounds like that should be fixed upstream, not a fan of peering into internals of other packages to fix their bugs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes

@giordano giordano linked an issue May 31, 2025 that may be closed by this pull request
@dalum
Copy link

dalum commented Jun 18, 2025

What is missing from this PR? Is any help needed?

@giordano
Copy link
Member

I'd prefer JuliaDiff/ForwardDiff.jl#665 to be resolved upstream rather than worked around here, so helping that PR would help this one in turn.

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.

promote_type(Measurement{Float64},BigFloat) fails
3 participants