-
Notifications
You must be signed in to change notification settings - Fork 4
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
Optimise DynamicPPL Slightly and Better Zero Adjoint Functionality #242
Conversation
Codecov ReportAttention: Patch coverage is
|
Performance Ratio:
|
CI was passing before I pushed a docstring tweak, so I should be fine to merge this in an hour or so. |
As a rule of thumb, we would like |
It's already in there -- see the diff associated to this PR :) |
This PR has now bloated to also tackle #241 , and #243 .
Ideally I would have tackled this in three separate PRs, but I got carried away fixing things.
So, it does several things:
simple_zero_adjoint
, which is useful to create rrules for things which don't need differentiating throughsimple_zero_adjoint
everywhere that we can in the code base -- there are quite a number of instances in which it workssimple_zero_adjoint
to add a DynamicPPL-specific rule, in an extension, to avoid some annoying computation that was adding overhead to Tapir.jl in that contextremove_dead_blocks(::BBCode)
, which will remove any basic blocks which cannot be reached. This was the solution to makingLKJCholesky
work properly. I don't fully understand what was going on, but basically I think that the compiler is making some assumptions about whatIRCode
it can see in practice, and I wasn't producing code which conformed to them.simple_zero_adjoint
rules for string and symbol related functionality that Tapir wasn't entirely happy with due to someccall
s.Tapir.TestUtils.test_rrule
and make use of them throughout the testsToDo:
remove_dead_blocks
I'm planning to finish this up on Monday.
edit: increased code churn is due to slow down in CI that we were observing due to not fully caching an interpreter for the current world age. This now does that, and some code has changed as a result. Additionally, the implementation of
_remove_unreachable_blocks
(renamed from_remove_dead_blocks
) has been heavily simplified, and the docstring associated to it improved substantially.