-
Notifications
You must be signed in to change notification settings - Fork 37
Add extra accs argument for LogDensityFunction #1196
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
2b67e2e to
99d53c6
Compare
ee6f77b to
c04612a
Compare
Benchmark Report
Computer InformationBenchmark Results |
|
DynamicPPL.jl documentation for PR #1196 is available at: |
c04612a to
7e63b61
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1196 +/- ##
=======================================
Coverage 78.99% 79.00%
=======================================
Files 41 41
Lines 3938 3939 +1
=======================================
+ Hits 3111 3112 +1
Misses 827 827 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
mhauru
left a comment
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.
LGTM
This PR fixes another feature regression in FastLDF, which is that the accumulators used for actual evaluation of the log-density are hardcoded. (Technically not hardcoded, but you have to overload some internal function, which is not good.)
In SlowLDF the accs were taken from the VarInfo passed in:
DynamicPPL.jl/src/logdensityfunction.jl
Lines 135 to 140 in 6c615ad
I found that I needed this functionality in the optimisation code so that I can pass in the accumulator that checks at runtime whether constraints are satisfied.
I think this isn't breaking, since it's the final positional argument and it is optional.