Remove selector/space stuff #2458
Conversation
Pull Request Test Coverage Report for Build 12713748848Details
💛 - Coveralls |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2458 +/- ##
==========================================
- Coverage 84.88% 81.60% -3.28%
==========================================
Files 21 21
Lines 1581 1506 -75
==========================================
- Hits 1342 1229 -113
- Misses 239 277 +38 ☔ View full report in Codecov by Sentry. |
|
I feel like intuitively I'd have removed it from DPPL first, is there a particular reason you started here? |
My thinking was to first stop using a feature of a dependency, then remove that feature. It doesn't make much of a difference, but I thought it might help make incremental changes. For instance, when making changes to DPPL, I should be able to test that Turing.jl tests pass with the new DPPL, without modifying Turing.jl code. |
There was a problem hiding this comment.
I suppose that the selector features in DPPL are almost purely motivated by this, so from that perspective it does make sense to me to remove this first. It's just a bit awkward with the testing machinery because you'd have to manually run the Turing testsuite with a dev'd version of DPPL..(?) Also, CI won't run the Turing integration tests.
Anyway, just some tiny tidyup suggestions below.
src/mcmc/particle_mcmc.jl
Outdated
| unset_flag!(vi, vn, "del") # Reference particle parent | ||
| r = rand(trng, dist) | ||
| vi[vn] = DynamicPPL.tovec(r) | ||
| DynamicPPL.setgid!(vi, spl.selector, vn) |
There was a problem hiding this comment.
Is this line needed? (Specifically the setgid one)
There was a problem hiding this comment.
Hmm, nice catch. I hope not. It shouldn't? I tried removing it, and a bunch of other calls to setgid! and updategid!, let's see what the test suite says.
| elseif length(proposals) == 1 | ||
| # If we hit this block, check to see if it's | ||
| # a run-of-the-mill proposal or covariance | ||
| # matrix. | ||
| prop = proposal(s) | ||
|
|
||
| # Return early, we got a covariance matrix. | ||
| return new{typeof(prop)}(prop) |
There was a problem hiding this comment.
This feels confusing to me, it could probably be handled better with a new method, but maybe a case for another time too.
There was a problem hiding this comment.
Is it the early return that confuses you? It confuses me. It's been there for at least ~5 years though, and I just left it be for now.
There was a problem hiding this comment.
Yeah, that and the call to proposal(), which is a bit opaque. But it's definitely out of scope for this PR.
Co-authored-by: Penelope Yong <[email protected]>
Co-authored-by: Penelope Yong <[email protected]>
This PR aims to remove all use of the
Selector/Gibbs ID/spacemechanisms that the old Gibbs sampler relied on. Methods for functions likegetspacewill still be defined for compatibility with the current DPPL version, but will not do anything. The goal is that after this has been merged a new release of DPPL could be made that removes the same mechanisms fromVarInfowithout anything breaking on the Turing.jl side.