-
Notifications
You must be signed in to change notification settings - Fork 37
Clean up matchingvalue etc #1190
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
4e8b93b to
45fe535
Compare
Benchmark Report
Computer InformationBenchmark Results |
7145038 to
82e9e2e
Compare
|
DynamicPPL.jl documentation for PR #1190 is available at: |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1190 +/- ##
==========================================
+ Coverage 78.91% 78.99% +0.08%
==========================================
Files 41 41
Lines 3927 3938 +11
==========================================
+ Hits 3099 3111 +12
+ Misses 828 827 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
3463e36 to
d0047d8
Compare
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
Closes #1090
Closes #796
This PR documents
matchingvalue, etc. and adds unit tests. It also renames the functions for clarity.It also fixes the behaviour in some edge cases, for example,
When running this with ForwardDiff AD, previously
Twould be promoted toVector{Dual{Float64}}, which was over-aggressively concrete. Now it is promoted toVector{Dual{Real}}.This is accomplished by using
Base.promote_typerather thanfloat_type_with_fallbackwhich is responsible for casting to Float64.Now,
float_type_with_fallback's only use is to generate the logp type in accumulators. It should also be renamed and tidied up (one could argue that it should be part of the accumulator API), but that's for a future PR.