-
Notifications
You must be signed in to change notification settings - Fork 35
Remove LogDensityProblemsAD Extension 2 #811
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
38fd931
to
90f62ca
Compare
5b05ad3
to
0e24d97
Compare
90f62ca
to
aef4b91
Compare
Benchmarks look good in the sense that they aren't any worse than on #806, and if anything, Mooncake is now actually a bit faster (maybe having fewer structs and stuff to differentiate through helps? I don't know):
|
8c98a73
to
892b097
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## py/no-ldp-ad #811 +/- ##
================================================
+ Coverage 85.79% 85.83% +0.03%
================================================
Files 35 35
Lines 4197 4200 +3
================================================
+ Hits 3601 3605 +4
+ Misses 596 595 -1 ☔ View full report in Codecov by Sentry. |
Since tests are passing, and the perf is equivalent / better to before, I'm going to merge this into the other PR. |
09ac46e
to
e4cd311
Compare
* Remove LogDensityProblemsAD * Implement LogDensityFunctionWithGrad in place of ADgradient * Dynamically decide whether to use closure vs constant * Combine LogDensityFunction{,WithGrad} into one (#811) * Warn if unsupported AD type is used * Update changelog * Update DI compat bound Co-authored-by: Guillaume Dalle <22795598+gdalle@users.noreply.github.com> * Don't store with_closure inside LogDensityFunction Co-authored-by: Guillaume Dalle <22795598+gdalle@users.noreply.github.com> * setadtype --> LogDensityFunction * Re-add ForwardDiffExt (including tests) * Add more tests for coverage --------- Co-authored-by: Guillaume Dalle <22795598+gdalle@users.noreply.github.com>
In this comment #806 (comment) I wrote:
This PR implements this. Benchmarks to follow.