-
Notifications
You must be signed in to change notification settings - Fork 42
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
base: master
Are you sure you want to change the base?
Conversation
Codecov ReportAttention: Patch coverage is
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. 🚀 New features to boost your workflow:
|
errors in nightly seem unrelated |
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 🙂 |
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 |
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). |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes
What is missing from this PR? Is any help needed? |
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. |
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
andForwardDiff.Dual isa Real
).What is missing is some methods defined exclusively forI added the definitions forMeasurements
((likeMeasurements.value
andMeasurements.measurement
), as well as constructor functions, but that discussion and PR can come later.value
/uncertainty
(just map those over the partials), andmeasurement
(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