Skip to content
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

Add IF2 algorithm #133

Merged
merged 32 commits into from May 19, 2021
Merged

Add IF2 algorithm #133

merged 32 commits into from May 19, 2021

Conversation

johnlees
Copy link
Member

@johnlees johnlees commented May 14, 2021

See:

TODO:

  • Make an IF2 control class
  • Add full set of tests for IF2 parameter class
  • Add full set of tests for IF2 class
  • Add full set of tests for IF2 control class
  • Docstrings (including examples)

Fixes #123

@codecov
Copy link

codecov bot commented May 14, 2021

Codecov Report

Merging #133 (d39737f) into master (a252242) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##            master      #133    +/-   ##
==========================================
  Coverage   100.00%   100.00%            
==========================================
  Files           21        24     +3     
  Lines         2140      2291   +151     
==========================================
+ Hits          2140      2291   +151     
Impacted Files Coverage Δ
R/smc2_parameters.R 100.00% <ø> (ø)
R/if2.R 100.00% <100.00%> (ø)
R/if2_control.R 100.00% <100.00%> (ø)
R/if2_parameters.R 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a252242...d39737f. Read the comment docs.

@johnlees johnlees marked this pull request as ready for review May 17, 2021 13:11
@johnlees johnlees requested a review from richfitz May 17, 2021 13:16
Copy link
Member

@richfitz richfitz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good so far - as you say it's not a massive piece of implementation. Or perhaps we're both broken by the cuda stuff.

In addition to the comments, could you add a vignette, no matter how brief, showing using this on either the sir or volatility model?

R/if2.R Outdated Show resolved Hide resolved
R/if2.R Outdated Show resolved Hide resolved
R/if2.R Outdated Show resolved Hide resolved
R/smc2_parameters.R Show resolved Hide resolved
tests/testthat/test-if2.R Show resolved Hide resolved
R/if2.R Outdated
##' # each final parameter estimate
##' n_particles <- 100
##' ll_samples <- filter$sample(n_particles)
if2 <- R6::R6Class(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is quite a different interface to that presented by pmcmc and I'd like to harmonise if possible. There, we have the functional interface where calling the function pmcmc returns a plain list of class mcstate_pmcmc which has pars, likelihoods etc. On the way it uses a semi-exported object pmcmc_state but that's an implementation detail.

So here, the if2 function should use the object much as you have it, but return all the requested information as a list. Unless there's some reason that's not possible, in which case please let me know.

R/if2.R Show resolved Hide resolved
R/if2.R Show resolved Hide resolved
@johnlees johnlees requested a review from richfitz May 17, 2021 16:43
@richfitz richfitz merged commit 4d065bb into master May 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement IF2 algorithm
2 participants