-
Notifications
You must be signed in to change notification settings - Fork 32
Shift most test from FiniteDiff to ForwardDiff #35
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
Conversation
|
Any estimates for difference in eval time with ForwardDiff vs FiniteDiff? |
|
Haven't checked yet in detail, feels a bit faster, but I could also be completely wrong |
|
Julia v1.8 (beta) shows timing information in tests as well. So possibly of interest. Current master: This PR: |
|
Thanks for the PR Jan! @rejuvyesh what is the correct way to include dependencies for the tests? I was under the impression that now there should be a test/Project.toml file: https://pkgdocs.julialang.org/v1/creating-packages/#Test-specific-dependencies-in-Julia-1.2-and-above Also, what are your thoughts on FiniteDiff vs ForwardDiff for the tests? Ideally we would just support one. |
Yeah,
Main thing I am not fully sure about is why do you have to define custom gradient/jacobian functions if AD like ForwardDiff already works? I would assume for performance/efficiency? In that case best to compare against proper AD like ForwardDiff. However, if you see that you are defining gradients for functions where AD like ForwardDiff is not going to work, then supporting FiniteDiff makes sense. |
Codecov Report
@@ Coverage Diff @@
## main #35 +/- ##
==========================================
- Coverage 91.73% 91.61% -0.12%
==========================================
Files 87 87
Lines 4477 4295 -182
==========================================
- Hits 4107 3935 -172
+ Misses 370 360 -10
Continue to review full report at Codecov.
|
|
Thanks for the clarification @rejuvyesh ! I've readded the test Project.toml. |
No description provided.