metrumresearchgroup / bbr

R interface for model and project management
https://metrumresearchgroup.github.io/bbr/
Other
22 stars 2 forks source link

Add inheriting parameter estimates to vignettes #631

Closed barrettk closed 6 months ago

barrettk commented 6 months ago

Add section on inheriting parameter estimates to getting started vignette

barrettk commented 6 months ago

@seth127 I have been unable to render the vignette locally for some reason (not surprising though). Stepping through the blocks works with no issues, but it seemingly gets hung up when trying to copy the model file:

mod2 <- copy_model_from(mod1) %>%
  update_model_id()

image

kyleam commented 6 months ago

@barrettk I don't know offhand what the source of your error is, but an alternative that may help you side step the issue: render the site locally with pkgdown::build_site(). That errors becauseinherit_param_estimates is missing from _pkgdown.yml, but once that's resolved your changes render fine on my end.

barrettk commented 6 months ago

@kyleam Thought about that but was trying to figure out if we should be doing anything else in terms of setting up file paths. Ended up just following your suggestion to move forward and make sure it rendered correctly. I updated the pkgdown file in this PR as opposed to in the separate release PR, but let me know if that's an issue.

kyleam commented 6 months ago

Thought about that but was trying to figure out if we should be doing anything else in terms of setting up file paths.

I don't really know what you're referring to (I'm not even spotting where you describe how you're rendering the vignettes), but of course please investigate any problems. Does whatever approach you're using work before this PR? Then it would definitely seem worth looking into the breakage. If it's not a new problem, then it's less of a concern.

Ended up just following your suggestion to move forward and make sure it rendered correctly. I updated the pkgdown file in this PR as opposed to in the separate release PR, but let me know if that's an issue.

It's a fix for something that should have been done in the original PR and that blocks building the site to check the vignette change here, so sure I think it makes sense as part of this PR. (The reason the pkgdown index update is often done in the release PR isn't because it belongs there but instead because we often forget to do it in the original PR.)

barrettk commented 6 months ago

Does whatever approach you're using work before this PR? Then it would definitely seem worth looking into the breakage. If it's not a new problem, then it's less of a concern.

I was just clicking the "knit" button within the Rmarkdown document. It was failing without the recent changes, but I got the feeling you guys may either A) be rendering it differently, or B) have some environment variable set before rendering vignettes locally that I didnt. My Metworx disc is a little wonky, and my profile is not sourced during each R session (unless I manually do it), so im never surprised when things like this only happen for me.

kyleam commented 6 months ago

I was just clicking the "knit" button within the Rmarkdown document.

I see. If I open RStudio and hit that button (with "Knit directory" set to "Project Directory"), it renders without an error for me. That requires installing bbr into the environment, which as far as I know is expected.

Another way that works on my end: using devtools::build_rmd("vignettes/getting-started.Rmd") (no need to install bbr in that case).

It was failing without the recent changes [...]

Ah, good to know it's not due to a change here.

[...] but I got the feeling you guys may either A) be rendering it differently, or B) have some environment variable set before rendering vignettes locally that I didnt.

I probably don't represent the normal workflow, but fwiw I tend to use devtools::build_rmd() (outside of RStudio) to render and check the vignette. Rendering works for me without setting any additional options or environment variables.

kyleam commented 6 months ago

requested review from seth127 and kyleam

@seth127 Let me know if you prefer I take this, but otherwise I'll leave to you. Thanks.