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

Advanced documentation #6

Merged
merged 4 commits into from
Feb 14, 2025
Merged

Advanced documentation #6

merged 4 commits into from
Feb 14, 2025

Conversation

afmagee42
Copy link
Collaborator

This PR adds two more vignettes.

  • "Model adequacy" shows how the posterior predictive distribution can be used to examine whether the model fits well
  • "Model details" describes some implementation decisions, some math, and a few sharp edges we encountered in setting this codebase up

@afmagee42 afmagee42 requested a review from cherz4 January 24, 2025 20:28
@afmagee42
Copy link
Collaborator Author

@cherz4 no rush on looking this over. I'd also say that while technically the scope here is just the two new vignettes, comments about the rest (added in #3) are welcome since these refer a lot to those.

@cherz4
Copy link

cherz4 commented Feb 10, 2025

Thanks @afmagee42!

I had a chance to do a deeper dive into all the code and vignettes of nbbp as I was preparing to address your PR. I'm happy to have had the opportunity to look at this - it's neat!

I will separate the below out into a single issue / document (vignette).

nbbp vignette (now Issue #7)

  • I was a little confused by the explanation of the likelihood surface plot and the second image that zoomed in on the ridge. What was the reader supposed to take away from this?
  • Fleshing out the R distribution overlap text in the measles comparison would be helpful for the reader to better follow your calculations.
  • I see no divergent transitions in the MERS-CoV example - is this text outdated (given you are running with a seed)?

Default Priors vignette (now Issue #8 ):

  • Suggest adding a short bit of text about why the lognormal and halfnormal distributions were selected for R and k.
  • Default prior on k: in the text did you mean to write k<= 10.0 instead of k<= 100?
  • There is no explanation for the third plot in the knitted html, Default prior predictive offspring distribution, suggest adding a brief description
  • Would be nice to see more code about how these priors were calculated/made/pulled out

Advanced Data vignette (now Issue #9)

  • In Treat it as non-extinct section, you refer to chainsR package, when I think you mean to refer to nbbp
  • You may wish to link to some Stan documentation on what divergences mean and when some amounts of divergence are reasonable or unreasonable. I am familiar with divergences, but the Stan message in your vignette is not the same as the one I worked with before (e.g. X # divergent transitions after warmup: How to Diagnose and Resolve Convergence Problems – Stan)
  • Can you explain / point out where you got the probability of 7% in the Comparison section more clearly?

@cherz4
Copy link

cherz4 commented Feb 10, 2025

I see you have two failing checks on this PR.

The pre-commit one I think is due to the template repo update for R setup that was resolved near the end of December 2024. You can see the fix here: https://github.com/cdcent/cfa-repo-template/commit/8487a48653bbfa7224986366e38d70833e13dc3b

I'm unclear if the Check R Packages / check-r-package (pull request) is related. Looking at the output of the failure there may also be some minor typos in 1-2 vignettes that are additionally causing this error - but it looks like you've tried to correct them with a commit on this PR. I'd suggest trying the above update to the pre-commit.yaml and seeing if it resolves both first.

@cherz4
Copy link

cherz4 commented Feb 10, 2025

For the vignettes on this PR:

Details vignette:
Overall LGTM with some nits:

  1. When viewed in HTML format the header sizes and spacing of sections could be improved a bit. I think a few headers may be bigger than intended and spacing between large sections could be added. You may wish to add a navigable table of contents (TOC) to your HTMLs for easier reader navigation.
  2. If you have time/interest, it would likely be informative to add more information and/or figures and/or output showing a specific example of the two methods (univariate profiling and parametric bootstrapping) when they are working and when they are not (eg produced CIs with poor coverage properties).

Model Adequacy vignette:
Overall: Need to fix 1 and 2 to run

  1. Currently an unresolved issue with object 'pool_predictive' that is preventing knitting / previewing this vignette in HTML. The original object 'posterior_predictive' appears to be appropriate and this code runs with that replacement.
  2. There are two instances of purrr functions needing to be explictly called with purrr:: and I have pointed these out in the comments.
  3. Once 1 and 2 are completed, the vignette can be knit and everything runs.
  4. A few typos I have pointed out in comments.
  5. You bring up the method of moments a few times in this vignette. It may be useful to link to an approachable intro on that method for the reader.
  6. It may help the reader to have at least a few more sentences (up to a paragraph or so) more context at the beginning for how one goes about assessing model accuracy.

@afmagee42 afmagee42 mentioned this pull request Feb 12, 2025
@afmagee42 afmagee42 requested a review from cherz4 February 13, 2025 15:47
@afmagee42
Copy link
Collaborator Author

Checks now passing after fixing both the action (#10) and the pointed-out vignette typos

Copy link

@cherz4 cherz4 left a comment

Choose a reason for hiding this comment

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

Overall LGTM. Consider opening issues on some of the other comments I made about these vignettes in the conversation thread (as you see fit!)

@afmagee42
Copy link
Collaborator Author

I've spun off #13, #15, and #16

@afmagee42 afmagee42 merged commit befc5d2 into main Feb 14, 2025
2 checks passed
@afmagee42 afmagee42 deleted the afm-more-docs branch February 14, 2025 15:11
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.

2 participants