submission: babette #209
Comments
|
This submission is a continuation of #208 |
Editor checks:
Editor commentsThanks for this (continued) submission, @richelbilderbeek! I have two initial pieces of feedback, neither of which preclude moving forward, so I'll go ahead and look for reviewers.
Reviewers: |
|
Thanks for the great feedback! Will do so at latest at 2018-04-16:
|
|
The example on tracerer README is broken since the function The The output of the example is while After looking at the source code, there is an option:
which by default is setting to 0, in the function Although by default it is expected to discard 0 tree, it actually discard the first one. Is this a bug or intended for some reasons? > rBEAST::beast2out.read.trees
function (file, opt.rescale.edge.length = 1, opt.burnin = 0)
{
tmp <- readLines(file, n = 2000, warn = FALSE)
tmp <- which(grepl("#NEXUS", tmp))
if (length(tmp) > 1) {
cat(paste("\nFound #NEXUS headers, n=", length(tmp),
".\nDiscard all lines before last entry on line",
tail(tmp, 1)))
cmd <- paste("sed -i\".bak\" 1,", tail(tmp, 1) - 1, "d ",
file, sep = "")
system(cmd)
cmd <- paste("sed -i\".bak2\" 1s/\\;// ", file, sep = "")
system(cmd)
cmd <- list.files(paste(rev(rev(strsplit(file, "/")[[1]])[-1]),
collapse = "/"), pattern = "*bak*", full.names = TRUE)
cat(paste("\nrm files\n", paste(cmd, collapse = "\n")))
file.remove(cmd)
}
mph <- ape::read.nexus(file)
tmp <- regexpr("[0-9]+", names(mph))
if (any(tmp < 0))
stop("unexpected nexus file without STATE iteration numbers")
mph.it <- as.numeric(regmatches(names(mph), tmp))
mph <- lapply(which(mph.it > opt.burnin), function(j) mph[[j]])
mph.it <- mph.it[mph.it > opt.burnin]
names(mph) <- paste("STATE_", mph.it, sep = "")
if (opt.rescale.edge.length != 1)
for (j in seq_along(mph)) mph[[j]]$edge.length <- mph[[j]]$edge.length *
opt.rescale.edge.length
mph
}The implementation of
### something like this in pure R is more favorable.
con <- readLines(file, n = 2000, warn = FALSE)
tmp <- which(grepl("#NEXUS", con))
if (length(tmp) > 1) {
cat(paste("\nFound #NEXUS headers, n=", length(tmp),
".\nDiscard all lines before last entry on line",
tail(tmp, 1)))
########
con <- con[tmp[length(tmp)]:length(con)]
}
mph <- ape::read.nexus(textConnection(con))
########
... |
|
Thanks @GuangchuangYu! @richelbilderbeek, for clarification, Guangchuang wasn't able to do a full review, but posted this contribution anyway - the benefit of open review is that anyone from the community can contribute! I'm still hunting for full reviewers. |
|
Thanks for your review @GuangchuangYu!
EDIT: I use BEAST
|
|
Added AppVeyor continuous integration, used |
|
First reviewer: @dwinter |
|
Second reviewer: @bjoelle |
|
It took a bit to get reviewers, @richelbilderbeek, but the search made it clear that there is a lot of interest in these packages! Like Guangchuang, there were several folks who had short comments but were not able to do a full review. One comment that came up is that one might want to use the xml2 package to generate and validate XML inputs rather than manipulating strings. I suggest that the reviewers consider this and determine whether they agree. |
|
The reason of not using |
|
There are some requests by reviewers of the manuscript of @noamross : do I have permission to change this function's name while under code review? Of course, all usages of it will also be updated and tested. References
|
|
@richelbilderbeek That's fine, and is in line with our own guidelines. |
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:
Functionality
Final approval (post-review)
Estimated hours spent reviewing: 25-30 Review CommentsThis package and its dependencies beautier, beastier and tracerer provide a R interface for setting up a BEAST2 analysis, running it and processing the results. I think this interface is a good alternative to the existing graphical interfaces, especially BEAUti which requires a lot of manipulations to set up even a simple analysis. In particular these packages will be very useful for people who want to integrate BEAST2 in an existing R pipeline. Overall I think the packages are well done, although the documentation is lacking in some places. All the packages are missing package documentation (required by rOpenSci guidelines). Regarding the suggestion about using xml2 instead of strings, the only real advantage I see to it is making it possible to load BEAST2 configuration files to modify or add an element instead of having to rebuild the model in babette, which might help/encourage people to transition to using babette. I am happy to leave the decision to the author on that. InstallationThe installation worked fine on Mac and Windows. babette
beautierStructure
Functionality
Documentation
Minor comments
beastier
tracerer
|
|
Thank you for the thorough review, @bjoelle! FYI @richelbilderbeek, the second reviewer requested an additional week to finish, so expect the next review next week. |
|
I am impressed by @bjoelle's review, thanks so much! I will happily implement that feedback. Sure, I will wait after the other reviewer is finished, to do so in one go. |
|
@noamross how is the state of the second review (as it is overdue)? I don't want to rush our beloved volunteers; I just want to know for sure nothing ended up in a state of limbo. |
|
Hi all, the outstanding review is mine and it is definitely on it's way. Apologies for the delay, I will try to get something by the end of the week. |
|
All beastier feedback has been addressed. Only beautier (the biggest!) to go... 🌈 |
|
Worked on this September 14th... just to let you know that there is progress... |
|
All feedback has been addressed, last (and biggest) part was about beautier. Rounding things off now, then will let know. |
|
@richelbilderbeek I've followed up with both reviewers and they are reviewing your changes. |
|
@richelbilderbeek @noamross I have reviewed the changes. One additional thing I would suggest would be to include the overwrite parameter in |
|
I have a chance now to check out the changes to all these packages. I want to say @richelbilderbeek should be commended for the work he has done to take our comments on board and improve the packages. I particularly like the way he has used the issue tracker to explain the rationale for changes that don't' exactly match the reviewer's suggestions. I think this is an excellent suite of packages that will support reproducible approaches in an important field, so I'm happy to recommend it be approved. |
|
Approved! Thanks @bjoelle and @dwinter for your thorough reviews and follow-ups, and @richelbilderbeek for your diligent work. It's been a long road and a lot of code to go through and I'm glad to have this suite as part of rOpenSci. To-dos:
Should you want to acknowledge your reviewers in your package DESCRIPTION, you can do so by making them Welcome aboard! We'd also love a blog post about your packages, either a short-form intro to it (https://ropensci.org/tech-notes/) or long-form post with more narrative about its development. (https://ropensci.org/blog/). If you are interested, @stefaniebutland will be in touch about content and timing. We've started putting together a gitbook 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, the corresponding repo is here. |
|
Hi @noamross, I am in a dilemma here, as I want to remove some half-baked functionality. This will change the interface a lot: bbt_run <- function(
fasta_filename, # Was 'fasta_filenames' (plural)
tipdates_filename = NA, # Added one week ago
site_model, # Was 'site_models' (plural)
clock_model, # Was 'clock_models' (plural)
tree_prior, # Was 'tree_priors' (plural)
# posterior_crown_age = NA, # Remove
# rest as-is
)If rOpenSci is flexible in allowing me to change the interface, I will first round my to-dos. After that, I will modify the interface as above, before I submit the package to CRAN. Is that the preferred way to go? If no, what is? |
|
These changes are fine, and I suggest you make the changes, transfer, set the version, then submit to CRAN with the new URLs in DESCRIPTION, so that the accepted version is the same as what is on CRAN. (We are started to roll out versioned peer-review badges so that it is clear what the version was at acceptance.) You are free to continue to develop and make both small and big changes after acceptance, we have a section of our dev guide dedicated to ongoing changes and maintenance: https://ropensci.github.io/dev_guide/evolution.html |
|
Hello @richelbilderbeek and thank you for all your work on having
We would love to help get more eyes on your work. This link - https://ropensci.org/tags/software-peer-review/ - will give you examples of blog posts by authors of peer-reviewed packages so you can get an idea of the style and length you prefer. Technotes are here: https://ropensci.org/technotes/. Here are some technical and editorial guidelines for contributing a blog post: https://github.com/ropensci/roweb2#contributing-a-blog-post. If you are interested in contributing a blog post, please let me know what timing might work for you. Happy to answer any questions. |
|
@stefaniebutland: I will happily create a blog post and am already working on it... |
|
@richelbilderbeek Do you have an idea of when you might submit a draft for review? Then we can choose a potential publication date. |
|
@stefaniebutland: I can submit a draft before the Friday 25th of January, 19:00 (Amsterdam time zone, UTC +1). Would that work out? |
|
Sounds great! I'll mark my calendar and we can confirm a publication date at that time. Thank you. |
Summary
babetteprovides for a full workflow alternative of four differentGUI and command-line tools.
https://github.com/richelbilderbeek/babette
[e.g., "data extraction, because the package parses a scientific data file format"]
reproducibility, as it provides a way to script three error-prone (and tedius) GUI
and one command-line tool.
Scientists that use the tool 'BEAST2' to do a Bayesian phylogenetic inference on DNA data.
yours differ or meet our criteria for best-in-category?
No
Requirements
Confirm each of the following by checking the box. This package:
Publication options
paper.mdmatching JOSS's requirements with a high-level description in the package root or ininst/.Detail
Does
R CMD check(ordevtools::check()) succeed? Paste and describe any errors or warnings:Does the package conform to rOpenSci packaging guidelines? Please describe any exceptions:
Deviation from this guideline
There is one point I deviate from this guideline:
When using roxygen2, add
#' @noRdto internal functions.There are two functions
beautier::default_params_docandbeastier::default_params_docthat are empty internal function. Their purpose is to have their parameters inherited.
Adding
#' @noRdstops internal and exported functions to inherit these parameters.If there is alternative, I'd be happy to hear it.
Deviate from
goodpractice::gp()There is one
goodpractice::gp()warning I ignore. About the 'Date' field inthe
DESCRIPTIONfile.goodpracticesuggest to remove it,R CMD check --as-cranforces to add it. I followed the CRAN standards,
If there is way to satisfy both checks, I'd love to hear it.
If this is a resubmission following rejection, please explain the change in circumstances:
If possible, please provide recommendations of reviewers - those with experience with similar packages and/or likely users of your package - and their GitHub user names:
Klaus Schliep, KlausVigo
Luke Harmon, lukejharmon
Jonathan Eastman, eastman
Joseph W. Brown, josephwb
The text was updated successfully, but these errors were encountered: