Thank you for all the work done on this package. I have looked over the changes made and have just a few additional notes before accepting your package. See the secondary review below and comment back here when you are ready for me to look it over again.
General package development
R CMD check
[x] SUGGESTION: Fix the global variable and function NOTES.
R CMD BiocCheck
[x] SUGGESTION: Fix the initialization NOTE.
We fixed as many as we could!
DESCRIPTION
[x] REQUIRED: I asked that you clarify that certain packages should be listed as an 'Import' even though they were not listed in NAMESPACE. I was able to locate instances of 'S4Vectors', 'shiny' and 'vegan' within the Shiny R code but I still cannot locate where 'tibble', 'ape', or 'scales' are used. Please indicate where they are used or remove them from 'Imports'. _The tibble package is used in find_biomarker.R and has been added to its imports. The ape package is used in dimred_pcoa.R and has been added to its imports. The scales packaged is used in dimred_pca.R and dimredpcoa.R and has been added to its imports.
[x] REQUIRED: 'methods' and 'stats' where not added to 'Imports:' though you marked that they were. Please add them.
[x] SUGGESTION: Consider adding 'Coverage' to 'biocViews:' if applicable. Thanks for the suggestion. Added
NAMESPACE
[x] REQUIRED: Once 'methods' and 'stats' are added to 'Imports:' in DESCRIPTION file then importFrom("methods", "as") and importFrom("stats", "as.formula", "cor.test", "density", "kruskal.test", "model.matrix", "quantile", "t.test", "wilcox.test") should be added to NAMESPACE file.
data
[x] REQUIRED: Thank you for including documentation for the data in 'inst/script'. There needs to be documentation though for where 'animalcules.rds' and 'TB_example_dataset.rds' come from.
[x] REQUIRED: Where is 'toydata.rds' actually used? It is currently being used in the examples for R/upsample_counts._
vignette
[x] REQUIRED: Line 36, 'animalcules' should be lowercase even if it is at the beginning of a sentence. Fixed.
[x] REQUIRED: Line 31, 'devtools::loadall(".")' is not needed and should be removed. Fixed._
man pages
[x] REQUIRED: You mentioned that the 'MAE.Rd' documentation was moved to 'R/data.R' but this file is not present. Is it located some place else? _We decided to remove MAE.rda from the package because it was redundant. This resulted in no need for the /data and /data-raw directories as well. We do use MAE.rds however, which resides in /inst/extdata/MAE.rds and is documented in /inst/script/extdata.R (along with toydata.rds).
read_pathoscope_data
[x] REQUIRED: No runnable examples were added to this file. This function shouldn't have been exported, we have changed it to internal instead. It will eventually be integrated into and run only from the Shiny application.
R code
[x] CLARIFICATION: We recommend that vapply() be used instead of sapply() because problems arixe when the 'X' argument to sapply() has length 0. The return type is then a list() rather than a vector or array. There are only 3 instances of sapply() in your code: Fixed.
grep_tid.R, line 11
utils.R, line 45
utils.R, line 64
[x] REQUIRED: I see you made the seq_along() or seq_len() changes to your R code but this also needs to be reflected in the shiny R code. Please make the necessary changes. Fixed.
Hello @anfederico,
Thank you for all the work done on this package. I have looked over the changes made and have just a few additional notes before accepting your package. See the secondary review below and comment back here when you are ready for me to look it over again.
General package development
R CMD check
R CMD BiocCheck
We fixed as many as we could!
DESCRIPTION
[x] REQUIRED: I asked that you clarify that certain packages should be listed as an 'Import' even though they were not listed in NAMESPACE. I was able to locate instances of 'S4Vectors', 'shiny' and 'vegan' within the Shiny R code but I still cannot locate where 'tibble', 'ape', or 'scales' are used. Please indicate where they are used or remove them from 'Imports'. _The tibble package is used in find_biomarker.R and has been added to its imports. The ape package is used in dimred_pcoa.R and has been added to its imports. The scales packaged is used in dimred_pca.R and dimredpcoa.R and has been added to its imports.
[x] REQUIRED: 'methods' and 'stats' where not added to 'Imports:' though you marked that they were. Please add them.
[x] SUGGESTION: Consider adding 'Coverage' to 'biocViews:' if applicable. Thanks for the suggestion. Added
NAMESPACE
importFrom("methods", "as")andimportFrom("stats", "as.formula", "cor.test", "density", "kruskal.test", "model.matrix", "quantile", "t.test", "wilcox.test")should be added to NAMESPACE file.data
[x] REQUIRED: Thank you for including documentation for the data in 'inst/script'. There needs to be documentation though for where 'animalcules.rds' and 'TB_example_dataset.rds' come from.
[x] REQUIRED: Where is 'toydata.rds' actually used? It is currently being used in the examples for R/upsample_counts._
vignette
[x] REQUIRED: Line 36, 'animalcules' should be lowercase even if it is at the beginning of a sentence. Fixed.
[x] REQUIRED: Line 31, 'devtools::loadall(".")' is not needed and should be removed. Fixed._
man pages
read_pathoscope_data
[x] REQUIRED: No runnable examples were added to this file. This function shouldn't have been exported, we have changed it to internal instead. It will eventually be integrated into and run only from the Shiny application.
R code
[x] CLARIFICATION: We recommend that
vapply()be used instead ofsapply()because problems arixe when the 'X' argument tosapply()has length 0. The return type is then alist()rather than a vector or array. There are only 3 instances ofsapply()in your code: Fixed.[x] REQUIRED: I see you made the
seq_along()orseq_len()changes to your R code but this also needs to be reflected in the shiny R code. Please make the necessary changes. Fixed.Best, Kayla
Originally posted by @Kayla-Morrell in https://github.com/Bioconductor/Contributions/issues/1052#issuecomment-485467251