-
Notifications
You must be signed in to change notification settings - Fork 37
Use view whenever possible
#272
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
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
yebai
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.
@torfjelde Maybe add some benchmarking with a concrete example? Also, a test would be helpful.
Benchmarks I'm with you on (but I see you found #248 which is exactly what I was going to refer to). But regarding tests, shouldn't this be covered abundantly by the existing test suite? |
| maybe_view(x::Expr) = :($(DynamicPPL.maybe_unwrap_view)(@view($x))) | ||
|
|
||
| maybe_unwrap_view(x) = x | ||
| maybe_unwrap_view(x::SubArray{<:Any,0}) = x[1] |
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.
Maybe add a comment on why we need unwrap view here? Otherwise, happy to merge as-is.
|
Benchmarks are showing a tiny performance improvement, though the models we're currently benchmarking are probably not ideal for demonstrating the difference. Current releaseSetupusing BenchmarkTools, DynamicPPL, Distributions, Serializationimport DynamicPPLBenchmarks: time_model_def, make_suite, typed_code, weave_childModels
|
|
bors try |
tryBuild failed: |
|
bors try |
|
bors r+ |
|
👎 Rejected by code reviews |
|
bors r+ |
In the more recent Julia versions `view` is zero-overhead, so maybe we should be using them all over the place? There might also be some neat stuff we can do by accessing `parent` within the tilde-statements:) Co-authored-by: Hong Ge <[email protected]>
|
It needs a version-bump, but we can do this in a direct commit to master after bors is done 👍 |
|
Timed out. |
|
Merging this now as the previous bors test was successful. |
In the more recent Julia versions
viewis zero-overhead, so maybe we should be using them all over the place?There might also be some neat stuff we can do by accessing
parentwithin the tilde-statements:)