-
Notifications
You must be signed in to change notification settings - Fork 18
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 second-order derivative functions #122
Conversation
adb3ae6
to
d634408
Compare
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #122 +/- ##
==========================================
+ Coverage 82.94% 83.37% +0.42%
==========================================
Files 8 8
Lines 428 451 +23
==========================================
+ Hits 355 376 +21
- Misses 73 75 +2 ☔ View full report in Codecov by Sentry. |
I think the change is welcome here and coherent regardless of whether it makes it into ForwardDiff. However it probably needs tests too, which I guess is why the PR remains a draft. |
d634408
to
ea14c4e
Compare
@gdalle Thanks. Tests are now live and passing so this is ready for review |
Gentle bump @gdalle; can you be a reviewer in this PR? |
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.
Thanks for the contribution!
The backend-agnostic part looks good to me. I would have to read up on ForwardDiff tags to understand what you did in the relevant extension, so if someone else can review that part great, if not I'll do it too.
Any reason why you didn't add tests for Tracker
and RuleConfig
backends?
Co-authored-by: David Widmann <[email protected]>
@gdalle @devmotion Thanks for the reviews.
I simply omitted them because I saw that those backends don't have tests for EDIT: the |
As soon as I understand what's going on with the tags, it's good to merge on my end |
Co-authored-by: David Widmann <[email protected]>
Thanks. Can we get a new release with this feature? |
Draft PR for changes discussed in JuliaDiff/ForwardDiff.jl#678
Fixes #71.