Update r-markdown.Rmd #854
Conversation
Redraft aimed at clarifying when `override = FALSE` must be used.
|
Thanks! I will probably take a look at this next week when I have more time. |
Codecov Report
@@ Coverage Diff @@
## master #854 +/- ##
==========================================
- Coverage 88.19% 86.61% -1.59%
==========================================
Files 12 12
Lines 4218 4362 +144
==========================================
+ Hits 3720 3778 +58
- Misses 498 584 +86
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
There was a problem hiding this comment.
I actually had a few moments so I took a look now. I think this is an improvement, I only noticed two issues. One I will just go ahead and fix and the other relates to the use of the term "in-memory", for which I proposed an alternative that I'd like your feedback on.
I will also take another look at this next week (I'll be traveling the next few days) to see if I notice anything else.
vignettes/r-markdown.Rmd
Outdated
| in-memory `stanmodel`, which is assigned to a variable with the name given by | ||
| the `output.var` chunk option. For example: | ||
| Behind the scenes in each option, the engine compiles the model code in each chunk into an | ||
| in-memory model: a `stanmodel` if Rstan is being used, or a `CmdStanModel` in the CmdStanR case. The in-memory model is assigned to a variable with the name given by the `output.var` chunk option. |
There was a problem hiding this comment.
One subtlety is that CmdStanR creates a compiled executable that technically isn't in-memory (from R's perspective). The CmdStanModel object itself is in-memory (so what you say here isn't wrong), but to run Stan it runs the compiled executable outside of R and reads the results back in (unlike RStan which does everything in-memory in R via Rcpp). To avoid any confusion about this maybe we should just remove "in-memory" and instead say something like the following, which I hope gets the idea across:
Behind the scenes in each option, the engine compiles the model code in each chunk and creates an object that provides methods to run the model: a
stanmodelif Rstan is being used, or aCmdStanModelin the CmdStanR case. This model object is assigned to a variable with the name given by theoutput.varchunk option.
If you're ok with that change I can make it.
There was a problem hiding this comment.
I am entirely content with that change.
There was a problem hiding this comment.
Ok great, I just went ahead and made that change. I also just read through the updated vignette again and I think it's much clearer now than before. Thank you again for bringing this to our attention and making the edits.
Once all the unit tests pass (they got triggered again because of my edit) I will merge this pull request.
Redraft aimed at clarifying when
override = FALSEmust be used.Submission Checklist
Summary
Please describe the purpose of the pull request.
Copyright and Licensing
Please list the copyright holder for the work you are submitting
(this will be you or your assignee, such as a university or company):
INSERT COPYRIGHT HOLDER HERE
By submitting this pull request, the copyright holder is agreeing to
license the submitted work under the following licenses: