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

Issue 289: Getting help with stan vignette #405

Merged
merged 71 commits into from Dec 13, 2023
Merged

Issue 289: Getting help with stan vignette #405

merged 71 commits into from Dec 13, 2023

Conversation

medewitt
Copy link
Collaborator

@medewitt medewitt commented Nov 29, 2023

Description

Getting the discussion on the PR to draft mode for future discussion. This PR closes #289.

This PR includes the initial draft for users getting help with Stan. It provides some extended information on the following:

  • Getting Stan and dependencies installed (toolchain)
  • How to manipulate Stan options through the epinowcast functions
  • Approaches to address common failure modes and error messages thrown by Stan

Checklist

  • My PR is based on a package issue and I have explicitly linked it.
  • I have included the target issue or issues in the PR title in the for Issue(s) issue-numbers: PR title
  • I have read the contribution guidelines.
  • I have tested my changes locally.
  • I have added or updated unit tests where necessary.
  • I have updated the documentation if required.
  • My code follows the established coding standards.
  • I have added a news item linked to this PR.
  • I have reviewed CI checks for this PR and addressed them as far as I am able.

medewitt and others added 11 commits November 21, 2023 06:12
…stan` syntax. (#386)

* first draft action that installs cmdstan

* use the correct approach to checking for stan changes

* add check for cmdstanr compilation and syntax correctness

* add skeleton news item

* update PR number

* use pull request not PR

* Automatic readme update [ci skip]

* make sure to install epinowcast

* missing testthat call

* Automatic readme update [ci skip]

* remove spurious readme figures

* add comments for cmdstan workflow

* make the refex match exactly only

* try a simpler approach to error catching

* fail around more

---------

Co-authored-by: GitHub Action <action@github.com>
@medewitt
Copy link
Collaborator Author

Tagging @zsusswein, too.

I wanted to get this in front of everyone. Happy for feedback (i.e., take a machete to it). I'm happy to edit/expand/etc after feedback.

I feel like it needs some examples using synthetic data which I can make (e.g., put in too little data and let the model fail, put in multi-modal data and let the posteriors be wide and ESS low).

Copy link

codecov bot commented Nov 29, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (b273412) 96.84% compared to head (41d45c4) 96.84%.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #405   +/-   ##
=======================================
  Coverage   96.84%   96.84%           
=======================================
  Files          15       15           
  Lines        1873     1873           
=======================================
  Hits         1814     1814           
  Misses         59       59           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Collaborator

@seabbs seabbs left a comment

Choose a reason for hiding this comment

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

This is so good!

A few comments mostly discussing my personal quest for validation via control of spacing. I think we could link out to more detail on some of these topics (like adapt delta settings etc) a bit more liberally and we might also want to give some tips for working with cmdstan objects (like tidybayes, posterior etc.).

I think we should aim to not make this PR review into what we would ultimately like in this vignette but instead aim for a good skeleton we can add to over time.

I feel like it needs some examples using synthetic data which I can make (e.g., put in too little data and let the model fail, put in multi-modal data and let the posteriors be wide and ESS low).

I think this would be lovely but would also be very happy with a first version without this and save adding for a later PR?

vignettes/stan-help.Rmd Outdated Show resolved Hide resolved
vignettes/stan-help.Rmd Show resolved Hide resolved
vignettes/stan-help.Rmd Outdated Show resolved Hide resolved
vignettes/stan-help.Rmd Outdated Show resolved Hide resolved
vignettes/stan-help.Rmd Outdated Show resolved Hide resolved
vignettes/stan-help.Rmd Outdated Show resolved Hide resolved
vignettes/stan-help.Rmd Outdated Show resolved Hide resolved
vignettes/stan-help.Rmd Outdated Show resolved Hide resolved
vignettes/stan-help.Rmd Show resolved Hide resolved
vignettes/stan-help.Rmd Outdated Show resolved Hide resolved
Co-authored-by: Sam Abbott <s.e.abbott12@gmail.com>
Copy link
Contributor

@zsusswein zsusswein left a 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! I'm new to {epinowcast} myself and found this a helpful guide. I left a bunch of comments inline -- they're split between minor typos and some suggestions around content to expand on or re-organize. I would consider them all optional and nitpicky.

One other thought: I would personally find it useful if this document mentioned some particular parameters to investigate in problematic fits. I don't know what the hierarchical variance parameters are called (or other frequent trouble spots), but it would speed up my model debugging to know where to look first. That may be out of scope, but thought it was worth mentioning.

vignettes/stan-help.Rmd Outdated Show resolved Hide resolved
vignettes/stan-help.Rmd Outdated Show resolved Hide resolved
vignettes/stan-help.Rmd Outdated Show resolved Hide resolved
vignettes/stan-help.Rmd Outdated Show resolved Hide resolved
vignettes/stan-help.Rmd Show resolved Hide resolved
vignettes/stan-help.Rmd Outdated Show resolved Hide resolved
vignettes/stan-help.Rmd Outdated Show resolved Hide resolved
vignettes/stan-help.Rmd Outdated Show resolved Hide resolved
vignettes/stan-help.Rmd Outdated Show resolved Hide resolved
vignettes/stan-help.Rmd Outdated Show resolved Hide resolved
@seabbs
Copy link
Collaborator

seabbs commented Dec 1, 2023

Nice comments @zsusswein.

One other thought: I would personally find it useful if this document mentioned some particular parameters to investigate in problematic fits. I don't know what the hierarchical variance parameters are called (or other frequent trouble spots), but it would speed up my model debugging to know where to look first. That may be out of scope, but thought it was worth mentioning.

I think we should make this out of scope for this PR but open an issue to add to this vignette (or possibly to expand into more than one if getting long) as I agree it would be very useful.

Part of the reason I think its out of scope is I think this might need #349 to be completed first otherwise its quite reliant on folk wisdom.

vignettes/stan-help.Rmd Outdated Show resolved Hide resolved
@seabbs
Copy link
Collaborator

seabbs commented Dec 5, 2023

@medewitt would it be helpful to add some suggestions for any of the comments? I think all/most of the reviewers would be happy to help so just give them/me a ping if useful!

@medewitt
Copy link
Collaborator Author

medewitt commented Dec 5, 2023

@medewitt would it be helpful to add some suggestions for any of the comments? I think all/most of the reviewers would be happy to help so just give them/me a ping if useful!

Sure! Sorry, I got a bit busier this weekend than planned. I should be able to pick this up Wednesday/Thursday this week....

medewitt and others added 4 commits December 5, 2023 16:49
Co-authored-by: Carl A. B. Pearson <pearsonca@users.noreply.github.com>
Co-authored-by: Zachary Susswein <46581799+zsusswein@users.noreply.github.com>
@seabbs seabbs self-requested a review December 10, 2023 17:45
Copy link
Collaborator

@seabbs seabbs left a comment

Choose a reason for hiding this comment

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

This looking even better now. A few more comments.

vignettes/stan-help.Rmd Outdated Show resolved Hide resolved
vignettes/stan-help.Rmd Outdated Show resolved Hide resolved
vignettes/stan-help.Rmd Show resolved Hide resolved
vignettes/stan-help.Rmd Outdated Show resolved Hide resolved
vignettes/stan-help.Rmd Outdated Show resolved Hide resolved
@medewitt
Copy link
Collaborator Author

@seabbs almost there. I'll fix the linting error ((default_priors <- enw_reference(data = enw_example("preprocessed"))) to separate lines).

If you feel good about it, I'll add a news item and move it out of draft.

@seabbs
Copy link
Collaborator

seabbs commented Dec 12, 2023

Awesome - I do indeed feel good about it!

@medewitt medewitt marked this pull request as ready for review December 13, 2023 13:17
@medewitt medewitt requested a review from seabbs December 13, 2023 13:21
Copy link
Collaborator

@seabbs seabbs left a 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. Thanks for putting so much effort into it @medewitt! I think you've addressed all the reviewer points (thanks @zsusswein and @pearsonca!) so this is good to go from my perspective.

Note I adjusted the pkgdown articles section to split out help with epinowcast from case studies as I think this a little bit clearer.

Copy link
Collaborator

@seabbs seabbs left a 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. Thanks for putting so much effort into it @medewitt! I think you've addressed all the reviewer points (thanks @zsusswein and @pearsonca!) so this is good to go from my perspective.

Note I adjusted the pkgdown articles section to split out help with epinowcast from case studies as I think this a little bit clearer.

@seabbs seabbs disabled auto-merge December 13, 2023 14:49
@seabbs seabbs merged commit 88ace98 into main Dec 13, 2023
9 checks passed
@seabbs seabbs deleted the stan-help branch December 13, 2023 14:50
@seabbs seabbs mentioned this pull request Dec 13, 2023
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.

Getting help with stan vignette
7 participants