jagstargets: reproducible JAGS pipelines at scale #425
Comments
|
@ropensci-review-bot assign @noamross as editor |
|
Assigned! @noamross is now the editor |
|
Hi @wlandau! Running editor checks, I am able to fully test the package but have the following issue checking the package using This appears to occur the |
|
Sorry about that, should be fixed now. |
Editor checks:
Thank you will. All tests are running fine now, and your |
|
@ropensci-review-bot add @dill to reviewers |
|
@dill added to the reviewers list. Review due date is 2021-05-11. Thanks @dill for accepting to review! Please refer to our reviewer guide. |
|
hi @wlandau, I'm just getting started here and in installation I see that currently |
|
Thanks for reviewing, @dill. Development |
Package Review
DocumentationThe package includes all the following forms of documentation:
Functionality
I was using a 2020 MacBook Air (Intel) running macOS 10.15.7 (Catalina). Here is the output of I tried: remotes::install_github("wlandau/jagstargets", dependencies=TRUE)but (2021/04/29) So instead I installed with remotes::install_github("wlandau/targets", dependencies=TRUE)
remotes::install_github("ropensci/tarchetypes", dependencies=TRUE)
remotes::install_github("wlandau/jagstargets", dependencies=TRUE)This worked fine. 2021-05-06 I tried using
Estimated hours spent reviewing: 9 (this is high as it includes reading the rOpenSci review documentation)
Review CommentsFor some background on where I'm coming from with this review: I am a statistician, but I work primarily with ecologists (who tend to be pretty keen on JAGS and JAGS-like programming), so I have some experience with these kinds of workflows. I don't have experience with General commentsI realise this is probably a daunting number of comments. In general
|
|
Thanks for your review, @dill. Your feedback helped clean up the documentation and make it more accessible to new users.
That's odd, https://app.codecov.io/gh/wlandau/jagstargets and my local machines both report 100% coverage. Do you happen to know if any tests were skipped due to missing optional packages?
In ropensci/jagstargets@b6d12e3, I rewrote the introductory paragraph and moved the technical details to a later section of the README.
ropensci/jagstargets@6528f14 inserts more inline prose to explain the intent of the tests.
Sure, implemented in ropensci/jagstargets@885cce7.
I am not sure I fully understand, but ropensci/jagstargets@63ad892 does work a little harder to put the
ropensci/jagstargets@0ef2c23 rewords that section to try to be more helpful.
Added a mention of the intro vignette.
ropensci/jagstargets@c5af606 adds a comment near the beginning to clarify a bit about what I mean by "validation".
ropensci/jagstargets@c5af606 changes the model to have one slope and one intercept. My intention with the different model was to demonstrate that
Added in ropensci/jagstargets@057c47c.
Hopefully ropensci/jagstargets@586f75e clears that up.
From the documentation of #' The target names use the `name` argument as a prefix, and the individual
#' elements of `jags_files` appear in the suffixes where applicable.
#' As an example, the specific target objects returned by
#' `tar_jags(name = x, jags_files = "y.jags", ...)` returns a list
#' of `targets::tar_target()` objects:Still takes getting used to though, I know.
Yes, it is just a hash to distinguish among individual simulation replications. Dynamic branching in
Awesome! Thanks for sharing.
Added in ropensci/jagstargets@240d5cd.
ropensci/jagstargets@3a8fb67 adds a
All this has to do with how
I solve (2) by reading the actual lines of text of the JAGS model file in a separate target. A downstream MCMC target writes these lines to a new temporary JAGS model file and runs the temporary file instead of the user's original file. Because of the way file tracking works in
I hope this clarifies the roles of the "file" and "lines" targets. They do need to be separate, and I do think these names accurately describe what the targets do, even if the specifics are esoteric for most users. Happy to consider specific suggestions for alternative names if you have any. |
|
Thanks for the changes and explanations @wlandau! I pulled 3a8fb6775e1a1e635846f827664af152b007bef1 before addressing the points below.
Thanks also for the explanations of the JAGS compilation process vs. Stan. I've got a much better understanding of how and why that works now. I had a think about other names for "file" and "lines" and didn't come up with anything less ambiguous (e.g., "source" but that could be used for either option) that wasn't very ugly (e.g., "jagsfile"/"jagslines"). I guess I'm just thinking about a very specific use case where I might compare similar models fitted in JAGS vs Stan and how I can tell which bit of the model is which but I guess the solution there is naming the previous targets better rather than the template. I'm happy to let that drop! |
Thanks, that was a big hint. Hopefully ropensci/jagstargets@58621cb has better coverage.
Yeah, |
|
Re-running That's helpful extra info on name conflicts, thanks for that! |
|
@ropensci-review-bot add @ernestguevarra to reviewers |
|
@ernestguevarra added to the reviewers list. Review due date is 2021-06-15. Thanks @ernestguevarra for accepting to review! Please refer to our reviewer guide. |
ropensci-review-bot
added
3/reviewer(s)-assigned
and removed
2/seeking-reviewer(s)
labels
|
@ernestguevarra Your review was due 2021-06-15. Please let us know when you are able to get it in. |
Package ReviewPlease check off boxes as applicable, and elaborate in comments below. Your review is not limited to these topics, as described in the reviewer guide
DocumentationThe package includes all the following forms of documentation:
On running the workflow in the worked example in the vignette entitled "Bayesian simulation pipelines with jagstargets" that adds a target called coverage, I see the following errors: Error: callr subprocess failed: could not find function "%>%"Error: callr subprocess failed: could not find function "summarize"In the library(dplyr)to be able to get the updated workflow running. When I reviewed library(R2jags)
library(dplyr)
library(targets)
library(jagstargets)I am unsure if this is correct thinking on my part but I am guessing because # _targets.R
library(targets)
library(jagstargets)
library(dplyr)
generate_data <- function (n = 10L) {
beta <- stats::rnorm(n = 2, mean = 0, sd = 1)
x <- seq(from = -1, to = 1, length.out = n)
y <- stats::rnorm(n, beta[1] + x * beta[2], 1)
# Elements of .join_data get joined on to the .join_data column
# in the summary output next to the model parameters
# with the same names.
.join_data <- list(beta = beta)
list(n = n, x = x, y = y, .join_data = .join_data)
}
list(
tar_jags_rep_summary(
model,
"model.jags",
data = generate_data(),
parameters.to.save = "beta",
batches = 5, # Number of branch targets.
reps = 4, # Number of model reps per branch target.
variables = "beta",
summaries = list(
~posterior::quantile2(.x, probs = c(0.025, 0.975))
)
),
tar_target(
coverage,
model %>%
group_by(variable) %>%
summarize(
coverage = mean(q2.5 < .join_data & .join_data < q97.5),
.groups = "drop"
)
)
)I am a newbie with using targets and these are common errors that I encounter also and my usual approach will be to make a call within the targets workflow to load the package that is causing the error. Whether this is the correct approach, I am not sure but I am flagging it here as others who would follow the example may face the same errors I saw/faced.
Functionality
On install of But
On jagstargets Coverage: 14.46%
R/tar_jags_rep.R: 0.00%
R/utils_language.R: 0.00%
R/tar_jags.R: 1.72%
R/tar_jags_rep_summary.R: 2.44%
R/tar_jags_rep_dic.R: 2.63%
R/tar_jags_rep_draws.R: 2.63%
R/tar_jags_df.R: 100.00%
R/tar_jags_example_data.R: 100.00%
R/tar_jags_example_file.R: 100.00%
R/utils_assert.R: 100.00%
R/utils_data.R: 100.00%
R/utils_jags.R: 100.00%
R/utils_logic.R: 100.00%However,
The package is consistent with rOpenSci packaging guidelines and also equally importantly with the Estimated hours spent reviewing:
Review CommentsFirstly, I would like to apologise for the extreme delay in reviewing this package. I have been severely disorganised with my work and this has impacted on my ability to get to this review promptly. I thank @wlandau for his work on the I have made a few comments above in the appropriate review topics on some minor issues I encountered when installing the development version of the package, on following one example in the simulation vignette, and on code coverage. In addition, on Good Practice check, I see the following result: It is good practice to
✖ write unit tests for all functions, and all package code in general. 14% of code lines are covered by
test cases.
R/tar_jags_rep_dic.R:100:NA
R/tar_jags_rep_dic.R:101:NA
R/tar_jags_rep_dic.R:102:NA
R/tar_jags_rep_dic.R:103:NA
R/tar_jags_rep_dic.R:104:NA
... and 557 more lines
✖ fix this R CMD check WARNING: LaTeX errors when creating PDF version. This typically indicates Rd
problems. LaTeX errors found:The first point above reflects the result I am seeing on code coverage check. For the second point, I am unsure where this is arising from. I do not get any warnings on I see no spelling issues after running I would also like to mention that there were few and very minor edits that can be made in the vignettes (missing words, punctuation) that don't impact on readability and comprehension of the material but the author might want to fully correct/edit. If so, I would be happy to make those edits into a pull request. Thank you again to @wlandau for your great work on this package and for the whole |
|
Thank you for your review, @ernestguevarra.
ropensci/jagstargets@30e4ef6 repairs the (Knowing what I know now after implementing Target Markdown, I think I could hack a custom
This is because
Some tests use
Often this particular error has something to do with the local LaTeX installation, e.g. https://stackoverflow.com/questions/10819959/diagnosing-r-package-build-warning-latex-errors-when-creating-pdf-version. I was unable to reproduce it locally.
Happy to accept pull requests about spelling and grammar, or correct any specific spelling/grammar issues you identify. Please let me know if there is something I missed in my response. |
|
Hi everyone, it has been a while since I responded to @ernestguevarra's review. Is there something else I forgot to do? |
|
Hi @wlandau, I tend to expect a second response with changes incorporating both reviewers feedback, but I realize your workflow incorporated both responses and all edits. That said, @ernestguevarra and @dill, do @wlandau's changes address your comments? |
|
absolutely! |
|
Thanks, @dill! @ernestguevarra, is there anything else you would like me to work on? |
|
Hi @wlandau. Apologies. Yes, your comments above has responded to my feedback. Thanks for explaining those issues to me and good to know that you are aware and have actually addressed those even before my comments. I have nothing else to ask to be worked on. Thank you again for your work on this package! |
|
@ropensci-review-bot approve jagstargets |
|
Approved! Thanks @wlandau for submitting and @dill, @ernestguevarra for your reviews! 😁 To-dos:
Should you want to acknowledge your reviewers in your package DESCRIPTION, you can do so by making them Welcome aboard! We'd love to host a post about your package - either a short introduction to it with an example for a technical audience or a longer post with some narrative about its development or something you learned, and an example of its use for a broader readership. If you are interested, consult the blog guide, and tag @stefaniebutland in your reply. She will get in touch about timing and can answer any questions. We maintain an online book with our best practice and tips, this chapter starts the 3d section that's about guidance for after onboarding. Please tell us what could be improved. Last but not least, you can volunteer as a reviewer via filling a short form. |
|
Thanks so much @noamross, @dill, and @ernestguevarra! I transferred the repo. @noamross, would you grant me repo admin permissions? I seem to have lost them with the transfer. @ernestguevarra, would you like to be included as a reviewer like @dill? https://github.com/ropensci/jagstargets/blob/e27d1ef3f260a41d005ed81d8b99e2364099400f/DESCRIPTION#L28-L33. Wondering because "Should the author(s) deem it appropriate, I agree to be acknowledged as a package reviewer ("rev" role) in the package DESCRIPTION file" was unchecked in your review. |
|
You should have repo permissions now, @wlandau . |
|
Thanks Noam! |
|
Looks like the |
Date accepted: 2021-09-27
Due date for @dill: 2021-05-11Submitting Author: Will Landau (@wlandau)
Repository: https://github.com/wlandau/jagstargets
Version submitted: 0.0.0.9000
Editor: @noamross
Reviewers: @dill, @ernestguevarra
Due date for @ernestguevarra: 2021-06-15
Archive: TBD
Version accepted: TBD
Scope
Please indicate which category or categories from our package fit policies this package falls under: (Please check an appropriate box below. If you are unsure, we suggest you make a pre-submission inquiry.):
Explain how and why the package falls under these categories (briefly, 1-2 sentences):
jagstargetsleverages the existing workflow automation capabilities oftargetsto orchestrate computation and skip up-to-date tasks in Bayesian data analysis pipelines.jagstargetsreduces the burden of user-side custom code thattargetswould otherwise require, which helps free statisticians to focus more on the models and less on the software engineering.jagstargetsis for Bayesian statisticians who develop and run JAGS models. Example workflows range from individual analyses of clinical data to large-scale simulation-based calibration studies for validation.targetsalready provides the same kind of workflow automation, but it requires more custom code to set up a workflow.jagstargetsuses specialized domain knowledge to make this process easier. PackagesrjagsandR2jagsinterface with JAGS but provide little workflow automation.N/A
N/A
Technical checks
Confirm each of the following by checking the box.
This package:
The continuous integration is temporarily having some issues because(Should be fixed now.)tarchetypeswas just accepted to CRAN and the binaries are not yet available.Publication options
This package depends on
posterior, which is not yet on CRAN.Do you intend for this package to go on Bioconductor?
Do you wish to submit an Applications Article about your package to Methods in Ecology and Evolution? If so:
MEE Options
I have prepared a JOSS manuscript, and I intend to submit it if
jagstargetsclears rOpenSci software review.Code of conduct
The text was updated successfully, but these errors were encountered: