Bioconductor Review

[zktuong]

Bioconductor/Contributions#3680

General

• Remove LICENSE.md, it is not necessary to provide two license files, copyright info is already included in LICENSE.[ ] We prefer the master branch be clean of other application files like _pkgdown.yml. Please move the file to a different branch or remove it.

README

• Please update README.md to display installation instructions from Bioconductor. See Bioconductor package installation instructions on any current Bioconductor package landing page.

NAMESPACE

• You seem to be importing the miloR package as a whole, while also importing specific functions with importFrom. We encourage selective imports using importFrom than importing all using import.

Man pages

• Add a package-level man page (https://contributions.bioconductor.org/docs.html#package-level-documentation)

R code

• Wherever message() or warning() calls are used, consider adding a verbose = TRUE/FALSE argument to allow users to control whether progress messages are printed to the console.[ ] requireNamespace() is only necessary for packages listed under Suggests: in the DESCRIPTION file, as they are not automatically installed or loaded. If a package is in Imports:, it is ensured to be installed when the package is installed, so there is no need to check for its availability with requireNamespace(). Instead, you should import specific functions using @importFrom package function in the documentation or call them explicitly with package::function(). Please check all your calls to requireNamespace and remove those that are unnecessary:

R/./projectProbability.R:    requireNamespace("stats")
R/./constructMarkovChain.R:    requireNamespace("Matrix")
R/./miloUmap.R:    requireNamespace("miloR")
R/./miloUmap.R:    requireNamespace("igraph")
R/./vdjPseudobulk.R:    requireNamespace("stats")
R/./getPbs.R:    requireNamespace("rlang")
R/./getPbs.R:        requireNamespace("stats")
R/./getPbs.R:        requireNamespace("Matrix")
R/./check.R:    requireNamespace("methods")
R/./check.R:    requireNamespace("methods")

• We encourage the use the various apply family functions over for loops (https://contributions.bioconductor.org/r-code.html#code-syntax-and-efficiency). Here are some examples of functions that use for loops:

R/./projectProbability.R:    for (i in seq_len(n)) {
R/./projectProbability.R:        for (j in seq_len(n)) {
R/./constructMarkovChain.R:    for (i in seq_len(length(waypoints))) {
R/./maxMinSampling.R:    for (ind in colnames(data)) {
R/./maxMinSampling.R:        for (k in 2:no.iterations - 1) {
R/./vdjPseudobulk.R:        for (col_n in extract_cols) {
R/./getPbs.R:        for (anno_col in col_to_take) {
R/./terminalStateFromMarkovChain.R:    for (i in cells) {
R/./splitCTgene.R:            for (item in sublist) {

• To improve clarity and ensure the function’s behavior is explicit, make sure that all functions use return() to specify the value being returned. Some functions that do not explicitly return are: projectPseudotimeToCell, .maxMinSampling, miloUmap, .minMaxScale, etc.

Check

• Please address as many of the remaining BiocCheck notes as possible:

ℹ NOTE: The recommended function length is 50 lines or less. There are 4
functions greater than 50 lines.
The longest 5 functions are:
• setupVdjPseudobulk() (R/setupVdjPseudobulk.R): 262 lines
• ...
• markovProbability() (R/markovProbability.R): 70 lines
✔ Checking formatting of DESCRIPTION, NAMESPACE, man pages, R source, and vigne…
ℹ NOTE: Consider shorter lines; 267 lines (14%) are 80 characters long.
First few lines:
• R/constructMarkovChain.R#L74 aff <- exp(-(ids$x^2) / ([adaptive.st](http://adaptive.st/) ...
• ...
• vignettes/vignette_reproduce_original.rmd#L176 And that's it! We have
successfully infe ...
ℹ NOTE: Consider multiples of 4 spaces for line indents; 6 lines (0%) are not.
First few lines:
• R/vdjPseudobulk.R#L61 "v_call_ab ...
• ...
• R/vdjPseudobulk.R#L66 ), col_to_take ...
ℹ See https://contributions.bioconductor.org/r-code.html
ℹ See styler package: https://cran.r-project.org/package=styler as described in
the BiocCheck vignette.

Vignette

• It is not entirely clear which vignette users should refer to when getting started with the package. I assume vignette_from_scRepertoire.Rmd serves this purpose, as it includes a brief package description and installation instructions. To improve clarity, I recommend renaming this vignette to dandelionR.Rmd, making it more intuitive for users to access the introductory vignette with vignette("dandelionR").
• Style vignettes with BiocStyle and include table of contents (see https://contributions.bioconductor.org/docs.html#vignettes).
• No code chunks necessary for running the vignette should be hidden for the user. Please remove include=FALSE from the chunk loading the destiny package or, since it is being loaded later, remove this specific chunk.

vignette_from_scRepertoire.Rmd

• Update to display installation instructions from Bioconductor. See Bioconductor package installation instructions on any current Bioconductor package landing page.
• When you mention “Check out the other vignette for an example dataset” it would be useful to include the code for accessing it: vignette("vignette_reproduce_original")