Allow use of compiled compare in more places #192
Conversation
Codecov Report
@@ Coverage Diff @@
## master #192 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 28 28
Lines 2616 2638 +22
=========================================
+ Hits 2616 2638 +22
Continue to review full report at Codecov.
|
| @@ -31,28 +31,134 @@ particle_filter_state <- R6::R6Class( | |||
| min_log_likelihood = NULL, | |||
| support = NULL, | |||
|
|
|||
| run_compiled = function() { | |||
| step_r = function(step_index, partial = FALSE) { | |||
There was a problem hiding this comment.
Similar to the comment on the dust PR, what's the reason for separating deterministic and stochastic, rather than having a single step function?
There was a problem hiding this comment.
there's always a design trade-off here - one function with many if/elses or two functions with similar logic. Here the compiled and non-compiled versions are so different that the two functions approach feels much easier to follow the logic through. In particular the R step function is very complicated and has quite a few performance-critical tweaks
|
|
||
| step_compiled = function(step_index, partial = FALSE) { | ||
| if (partial) { | ||
| stop("'partial' not supported with compiled compare") |
There was a problem hiding this comment.
Maybe I'm confused, but I thought support was just added for this? Or am I misunderstanding what partial is?
There was a problem hiding this comment.
partial is the support we put in for (one of) if2/smc2, where you get the likelihood contributed by the last single step. We don't support that but we do support running to a point that is not the end. It's confusing yes :)
| pars <- list(exp_noise = Inf) | ||
| ## there's a discrepency here which ironically goes away with | ||
| ## non-infinite noise due to combination of sum over small numbers | ||
| ## and slightly different density calculation accuracy. | ||
| expect_equal(p1$run(pars), p2$run(pars), tolerance = 2e-5) |
There was a problem hiding this comment.
Different example/non-Inf noise simpler then?
There was a problem hiding this comment.
Using an Inf noise keeps the calculation deterministic, even if not quite at the accuracy we'd like. Getting the details out as to why this is slightly off would be potentially interesting but fairly sure that it's just that we have some poor accuracy in the densities when very unlikely (the R versions of these have lots of support to make these accurate but we can't use them due to thread safety)
Co-authored-by: John Lees <j.lees@imperial.ac.uk>
This reverts commit fffc0b2.
This PR allows use of the compiled compare function in a few more places, using support added in mrc-ide/dust#360
Merge mrc-ide/dust#360 first and unpin branch
Fixes #177