-
Notifications
You must be signed in to change notification settings - Fork 5
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
Issue 44: Approximate inference vignette #69
Conversation
Updates based on #70:
|
f9d6cf2
to
c799f97
Compare
@kgostic @seabbs @parksw3 could you give some midway feedback before I finish up this first version of the approximate inference vignette? My view of what needs to be done before a first merge:
My view of what can be left to a second try at improving this:
|
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.
Content so far looks okay. I assume its an initialisation issue for pathfinder. have you given multipathfinder a spin?
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.
Content so far looks okay. I assume its an initialisation issue for pathfinder. have you given multipathfinder a spin?
Thanks for the suggestion Sam, haven't tried it yet (though the algorithm did have four runs, and perhaps 1-2 of them had this error, so I may already have been using the "multi" version). To try r.e. fixing pathfinder (open to other suggestions!):
|
5640135
to
8f2be56
Compare
Steps needed to merge this are:
|
Do you actually want a review given outstanding tasks and failing CI or advice on something or? |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #69 +/- ##
===========================================
+ Coverage 28.49% 38.69% +10.20%
===========================================
Files 10 10
Lines 565 553 -12
===========================================
+ Hits 161 214 +53
+ Misses 404 339 -65 ☔ View full report in Codecov by Sentry. |
Hi @athowes thanks for writing this vignette. Really insightful and similar to what I'm writing for EpiNow2 here epiforecasts/EpiNow2#695. I'd be happy for you to review when it's ready and if you're interested. Two comments:
|
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.
One small typo noted but other than that this is looking good to me!
Thanks so much for reading @jamesmbaazam! I've added the units, and included your suggestion about moving some of the technical detail to an appendix in the follow-up issue about this vignette (#141). |
This looks amazing. I'm shocked at how fast other approximate methods run and how well they work... I found one typo.
|
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.
This is really nice now. Good to see all the love! I agree with the idea of moving some technical terms to an appendix.
* Laplace and pathfinder via brms * Update vignette outline and use S3 versions * Fix typo * Rename ltcad to latent_individual * Background and Laplace * Decent way through first draft now * Fix lint * Add required package deps * Improve text, figure, and note pathfinder error * Text and code improvements for approximate inference vignette * Sam comment * Better describe ADVI * Some writing, layout, and use of purrr * Add plot of delay distributions * Add time taken manually (looks like no inbuilt Stan functions for this) * What is Pathfinder (basic) * Standardise colours across figures * Clarify the stochastic behaviour of Pathfinder, set a seed that works, use single path * Use imap as suggested by Damon * Finish first draft of approximate inference vignette with conclusions, and pass lintr * Try adding back rstan to see if it fixes CI * Add description * Add GH remote for brms to get latest bug fix * Fix typos * As of recent PR then prepare has been replaced * Add units to times * Fix typo spotted by Katie * Fix typo spotted by Daniel --------- Co-authored-by: Sam Abbott <[email protected]> Former-commit-id: 18c34b36840e9458ed878ae6878a090bbcef6451 [formerly e636d4adf8f65526493281c7537bb3fe0bb7018a] Former-commit-id: 858ae8c2b9fa2fed9661f36e66a44fa47132b688
* Laplace and pathfinder via brms * Update vignette outline and use S3 versions * Fix typo * Rename ltcad to latent_individual * Background and Laplace * Decent way through first draft now * Fix lint * Add required package deps * Improve text, figure, and note pathfinder error * Text and code improvements for approximate inference vignette * Sam comment * Better describe ADVI * Some writing, layout, and use of purrr * Add plot of delay distributions * Add time taken manually (looks like no inbuilt Stan functions for this) * What is Pathfinder (basic) * Standardise colours across figures * Clarify the stochastic behaviour of Pathfinder, set a seed that works, use single path * Use imap as suggested by Damon * Finish first draft of approximate inference vignette with conclusions, and pass lintr * Try adding back rstan to see if it fixes CI * Add description * Add GH remote for brms to get latest bug fix * Fix typos * As of recent PR then prepare has been replaced * Add units to times * Fix typo spotted by Katie * Fix typo spotted by Daniel --------- Co-authored-by: Sam Abbott <[email protected]> Former-commit-id: 6b94acc Former-commit-id: f71a28b9813e55719fbff9743aa9007530861d9a
* Laplace and pathfinder via brms * Update vignette outline and use S3 versions * Fix typo * Rename ltcad to latent_individual * Background and Laplace * Decent way through first draft now * Fix lint * Add required package deps * Improve text, figure, and note pathfinder error * Text and code improvements for approximate inference vignette * Sam comment * Better describe ADVI * Some writing, layout, and use of purrr * Add plot of delay distributions * Add time taken manually (looks like no inbuilt Stan functions for this) * What is Pathfinder (basic) * Standardise colours across figures * Clarify the stochastic behaviour of Pathfinder, set a seed that works, use single path * Use imap as suggested by Damon * Finish first draft of approximate inference vignette with conclusions, and pass lintr * Try adding back rstan to see if it fixes CI * Add description * Add GH remote for brms to get latest bug fix * Fix typos * As of recent PR then prepare has been replaced * Add units to times * Fix typo spotted by Katie * Fix typo spotted by Daniel --------- Co-authored-by: Sam Abbott <[email protected]> Former-commit-id: 6b94acc Former-commit-id: f71a28b9813e55719fbff9743aa9007530861d9a
* Laplace and pathfinder via brms * Update vignette outline and use S3 versions * Fix typo * Rename ltcad to latent_individual * Background and Laplace * Decent way through first draft now * Fix lint * Add required package deps * Improve text, figure, and note pathfinder error * Text and code improvements for approximate inference vignette * Sam comment * Better describe ADVI * Some writing, layout, and use of purrr * Add plot of delay distributions * Add time taken manually (looks like no inbuilt Stan functions for this) * What is Pathfinder (basic) * Standardise colours across figures * Clarify the stochastic behaviour of Pathfinder, set a seed that works, use single path * Use imap as suggested by Damon * Finish first draft of approximate inference vignette with conclusions, and pass lintr * Try adding back rstan to see if it fixes CI * Add description * Add GH remote for brms to get latest bug fix * Fix typos * As of recent PR then prepare has been replaced * Add units to times * Fix typo spotted by Katie * Fix typo spotted by Daniel --------- Co-authored-by: Sam Abbott <[email protected]> Former-commit-id: 6b94acc Former-commit-id: f71a28b9813e55719fbff9743aa9007530861d9a
* Laplace and pathfinder via brms * Update vignette outline and use S3 versions * Fix typo * Rename ltcad to latent_individual * Background and Laplace * Decent way through first draft now * Fix lint * Add required package deps * Improve text, figure, and note pathfinder error * Text and code improvements for approximate inference vignette * Sam comment * Better describe ADVI * Some writing, layout, and use of purrr * Add plot of delay distributions * Add time taken manually (looks like no inbuilt Stan functions for this) * What is Pathfinder (basic) * Standardise colours across figures * Clarify the stochastic behaviour of Pathfinder, set a seed that works, use single path * Use imap as suggested by Damon * Finish first draft of approximate inference vignette with conclusions, and pass lintr * Try adding back rstan to see if it fixes CI * Add description * Add GH remote for brms to get latest bug fix * Fix typos * As of recent PR then prepare has been replaced * Add units to times * Fix typo spotted by Katie * Fix typo spotted by Daniel --------- Co-authored-by: Sam Abbott <[email protected]> Former-commit-id: 18c34b36840e9458ed878ae6878a090bbcef6451 [formerly e636d4adf8f65526493281c7537bb3fe0bb7018a] Former-commit-id: 858ae8c2b9fa2fed9661f36e66a44fa47132b688
* Laplace and pathfinder via brms * Update vignette outline and use S3 versions * Fix typo * Rename ltcad to latent_individual * Background and Laplace * Decent way through first draft now * Fix lint * Add required package deps * Improve text, figure, and note pathfinder error * Text and code improvements for approximate inference vignette * Sam comment * Better describe ADVI * Some writing, layout, and use of purrr * Add plot of delay distributions * Add time taken manually (looks like no inbuilt Stan functions for this) * What is Pathfinder (basic) * Standardise colours across figures * Clarify the stochastic behaviour of Pathfinder, set a seed that works, use single path * Use imap as suggested by Damon * Finish first draft of approximate inference vignette with conclusions, and pass lintr * Try adding back rstan to see if it fixes CI * Add description * Add GH remote for brms to get latest bug fix * Fix typos * As of recent PR then prepare has been replaced * Add units to times * Fix typo spotted by Katie * Fix typo spotted by Daniel --------- Co-authored-by: Sam Abbott <[email protected]> Former-commit-id: 18c34b36840e9458ed878ae6878a090bbcef6451 [formerly e636d4adf8f65526493281c7537bb3fe0bb7018a] Former-commit-id: 858ae8c2b9fa2fed9661f36e66a44fa47132b688 Former-commit-id: f5fd3f0
* Laplace and pathfinder via brms * Update vignette outline and use S3 versions * Fix typo * Rename ltcad to latent_individual * Background and Laplace * Decent way through first draft now * Fix lint * Add required package deps * Improve text, figure, and note pathfinder error * Text and code improvements for approximate inference vignette * Sam comment * Better describe ADVI * Some writing, layout, and use of purrr * Add plot of delay distributions * Add time taken manually (looks like no inbuilt Stan functions for this) * What is Pathfinder (basic) * Standardise colours across figures * Clarify the stochastic behaviour of Pathfinder, set a seed that works, use single path * Use imap as suggested by Damon * Finish first draft of approximate inference vignette with conclusions, and pass lintr * Try adding back rstan to see if it fixes CI * Add description * Add GH remote for brms to get latest bug fix * Fix typos * As of recent PR then prepare has been replaced * Add units to times * Fix typo spotted by Katie * Fix typo spotted by Daniel --------- Co-authored-by: Sam Abbott <[email protected]> Former-commit-id: 6b94acc Former-commit-id: f71a28b9813e55719fbff9743aa9007530861d9a Former-commit-id: f1ed373d7d4daaa0e1b3e33adeebcf258883ae30 [formerly 7ad3259] Former-commit-id: 47c4b62ddb622995066d3d1882001d37b087dc4c
* Laplace and pathfinder via brms * Update vignette outline and use S3 versions * Fix typo * Rename ltcad to latent_individual * Background and Laplace * Decent way through first draft now * Fix lint * Add required package deps * Improve text, figure, and note pathfinder error * Text and code improvements for approximate inference vignette * Sam comment * Better describe ADVI * Some writing, layout, and use of purrr * Add plot of delay distributions * Add time taken manually (looks like no inbuilt Stan functions for this) * What is Pathfinder (basic) * Standardise colours across figures * Clarify the stochastic behaviour of Pathfinder, set a seed that works, use single path * Use imap as suggested by Damon * Finish first draft of approximate inference vignette with conclusions, and pass lintr * Try adding back rstan to see if it fixes CI * Add description * Add GH remote for brms to get latest bug fix * Fix typos * As of recent PR then prepare has been replaced * Add units to times * Fix typo spotted by Katie * Fix typo spotted by Daniel --------- Co-authored-by: Sam Abbott <[email protected]> Former-commit-id: 6b94acc Former-commit-id: f71a28b9813e55719fbff9743aa9007530861d9a Former-commit-id: f1ed373d7d4daaa0e1b3e33adeebcf258883ae30 [formerly 7ad3259] Former-commit-id: 47c4b62ddb622995066d3d1882001d37b087dc4c
Description
This PR closes #44.
Extensions moved to issue: #141
Checklist