Closed armanshahrisa closed 6 years ago
Hi @armanshahrisa
Thanks for submitting your package. We are taking a quick look at it and you will hear back from us soon.
The DESCRIPTION file for this package is:
Package: cbaf
biocViews: Technology
Title: CBAF
Version: 0.99.1
Authors@R: c(person("Arman", "Shahrisa", role = c("aut", "cre", "cph"),
email = "shahrisa.arman@hotmail.com"),
person("Maryam", "Tahmasebi Birgani", role = "aut",
email = "tahmasebi-ma@ajums.ac.ir"))
Description: This package contains functions that allow analysing and comparing various
gene groups from different cancers/cancer subgroups easily. So far, it is
compatible with RNA-seq, microRNA-seq, microarray and methylation datasets
that are stored on cbioportal.org.
License: Artistic-2.0
Encoding: UTF-8
LazyData: true
Imports: BiocFileCache, RColorBrewer, cgdsr, genefilter, gplots, grDevices, stats, utils, xlsx
NeedsCompilation: no
RoxygenNote: 6.0.1
Suggests: knitr,
rmarkdown
VignetteBuilder: knitr
Collate:
'cbaf-obtainMultipleStudies.R'
'cbaf-obtainOneStudy.R'
'cbaf-automatedStatistics.R'
'cbaf-availableData.R'
'cbaf-heatmapOutput.R'
'cbaf-xlsxOutput.R'
'cbaf-processMultipleStudies.R'
'cbaf-processOneStudy.R'
Your package has been approved for building. Your package is now submitted to our queue.
IMPORTANT: Please read the instructions for setting up a push hook on your repository, or further changes to your repository will NOT trigger a new build.
Dear Package contributor,
This is the automated single package builder at bioconductor.org.
Your package has been built on Linux, Mac, and Windows.
On one or more platforms, the build results were: "skipped, TIMEOUT, ERROR". This may mean there is a problem with the package that you need to fix. Or it may mean that there is a problem with the build system itself.
Please see the following build report for more details:
http://bioconductor.org/spb_reports/cbaf_buildreport_20170818184859.html
Received a valid push; starting a build. Commits are:
6b88869 - Minor corrections
Dear Package contributor,
This is the automated single package builder at bioconductor.org.
Your package has been built on Linux, Mac, and Windows.
Congratulations! The package built without errors or warnings on all platforms.
Please see the following build report for more details:
http://bioconductor.org/spb_reports/cbaf_buildreport_20170821175221.html
Please revise your package in response to the following review, and post a summary of your changes when complete.
DESCRIPTION
vignettes
Please exclude built versions from your master branch, just include .Rmd file
The vignette contains 'pictures of code', but needs to contain fully evaluated code chunks, i.e., ```{r}...```. The vignette need to be processed in the time frame specified in the package guidelines.
R
format code to avoid long (>80 characters) lines, where possible
cbaf-automatedStatistics.R:292: if (continue == TRUE)
is a tautology, just if (continue)
stop(paste(...))
can usually be written stop(...)
(stop()
pastes its arguments together).
cbaf-automatedStatistics.R:313: Don't manipulate global options.
Use seq_len()
or seq_along()
rather than 1:...
Avoid highly nested calls, and repeated evaluation of identical expressions. This helps legibility and perfomance. E.g., cbaf-automatedStatistics.R:376
for(fp in 1:geneNumber){
# Check all members are under cutoff
if(!is.na(mean(as.vector(source.data.subset[,fp])[abs(source.data.subset[,fp])])) &
is.nan(mean(as.vector(source.data.subset[,fp])[abs(source.data.subset[,fp]) >= cutoff], na.rm=TRUE))){
...
might be
for(fp in seq_len(geneNumber)) {
# Check all members are under cutoff
value <- source.data.subset[,fp]
idx <- abs(value)
test1 <- mean(value[idx])
test2 <- mean(value[idx >= cutoff], na.rm=TRUE)
if (!is.na(test) & is.nan(test2)) {
...
Move function calls that are constant within a loop outside the loop, e.g,. cbaf-automatedStatistics.R:467
for(topV in 1:length(topGenes.values)){
...
colnames(complete.top) <- c(paste(topV, "th ", "Gene", sep=""), paste(topV, "th ", "Value", sep=""))
might be
colnames <- paste0(topV, "th ", c("Gene", "Value"))
for(topV in seq_along(topGenes.values)){
...
colnames(complete.top) <-colnames
avoid 'copy-and-append' where a vector is grown over iterations; use *apply()
or pre-allocate and fill, e.g., cbaf-automatedStatistics.R:338
for(gg in 1:length(sourceDataList)){
...
if(cs == 1){
temList$Top.Genes.of.Frequency.Percentage <- post.topGenes
} else if(cs!=1){
temList$Top.Genes.of.Frequency.Percentage <- rbind(temList$Top.Genes.of.Frequency.Percentage, post.topGenes)
}
might be
Top.Genes.of.Frequency.Percentage <- vector("list", length(sourceDataList))
for (gg in seq_along(sourceDataList)) {
...
Top.Genes.of.Frequency.Percentage[[g]] <- post.topGenes
}
temList$Top.Genes.of.Frequency.Percentage <-
do.call("rbind", Top.Genes.of.Frequency.Percentage)
your source code is 'double spaced', which makes it hard to visualize on a single screen.
never use assign()
to the global environment, e.g., cbaf-automatedStatistics.R:1083; return the value to the user or manage it in package-global space.
at cbaf-availableData.R:206, one branch of the conditional returns a logical vector, the other a character vector. Use consistent data types (logical, in this case) to simplify subsequent code.
use message()
, warning()
, and stop()
rather than print()
or cat()
to communicate with the user.
man
Received a valid push; starting a build. Commits are:
8ae43ad - Minor Improvements - Storing database in package...
Dear Package contributor,
This is the automated single package builder at bioconductor.org.
Your package has been built on Linux, Mac, and Windows.
On one or more platforms, the build results were: "skipped, ERROR". This may mean there is a problem with the package that you need to fix. Or it may mean that there is a problem with the build system itself.
Please see the following build report for more details:
http://bioconductor.org/spb_reports/cbaf_buildreport_20170909181806.html
Received a valid push; starting a build. Commits are:
61bc249 - version bump
Dear Package contributor,
This is the automated single package builder at bioconductor.org.
Your package has been built on Linux, Mac, and Windows.
On one or more platforms, the build results were: "ERROR". This may mean there is a problem with the package that you need to fix. Or it may mean that there is a problem with the build system itself.
Please see the following build report for more details:
http://bioconductor.org/spb_reports/cbaf_buildreport_20170909184747.html
Received a valid push; starting a build. Commits are:
9bc1a09 - Fixing an error - Destroying vignette to match b...
Dear Package contributor,
This is the automated single package builder at bioconductor.org.
Your package has been built on Linux, Mac, and Windows.
On one or more platforms, the build results were: "ERROR". This may mean there is a problem with the package that you need to fix. Or it may mean that there is a problem with the build system itself.
Please see the following build report for more details:
http://bioconductor.org/spb_reports/cbaf_buildreport_20170910065340.html
Received a valid push; starting a build. Commits are:
a731423 - Minor improvements
Dear Package contributor,
This is the automated single package builder at bioconductor.org.
Your package has been built on Linux, Mac, and Windows.
Congratulations! The package built without errors or warnings on all platforms.
Please see the following build report for more details:
http://bioconductor.org/spb_reports/cbaf_buildreport_20170910091058.html
Dear Dr. Martin Morgan,
I'm grateful for your informative points. The following is the list of what I did.
DESCRIPTION
vignettes
Vignette is stored as a Rmd file only.
Code chunks now use ```{r}...```
format.
R
I tried to limit the lines to 80 characters. (I have to admit that they look ugly and harder to read now :(( )
All the if (continue == TRUE)
were replaced with if (continue)
. Similar cases were also replaced, even when they contained FALSE instead of TRUE.
All stop(paste(...))
were simpified with stop(...)
.
Usage of stringsAsFactors
was transferred inside the corresponding data.frame() instead of changing the global option.
All the for(1 in 1:length())
were replaced with for(1 in seq_along())
.
Code was simplified in a similar manner as you mentioned:
Avoid highly nested calls, and repeated evaluation of identical expressions. This helps legibility and perfomance. E.g., cbaf-automatedStatistics.R:376
for(fp in 1:geneNumber){ # Check all members are under cutoff if(!is.na(mean(as.vector(source.data.subset[,fp])[abs(source.data.subset[,fp])])) & is.nan(mean(as.vector(source.data.subset[,fp])[abs(source.data.subset[,fp]) >= cutoff], na.rm=TRUE))){ ...
might be
for(fp in seq_len(geneNumber)) { # Check all members are under cutoff value <- source.data.subset[,fp] idx <- abs(value) test1 <- mean(value[idx]) test2 <- mean(value[idx >= cutoff], na.rm=TRUE) if (!is.na(test) & is.nan(test2)) { ...
topV
is filled by a for loop.Move function calls that are constant within a loop outside the loop, e.g,. cbaf-automatedStatistics.R:467
for(topV in 1:length(topGenes.values)){ ... colnames(complete.top) <- c(paste(topV, "th ", "Gene", sep=""), paste(topV, "th ", "Value", sep=""))
might be
colnames <- paste0(topV, "th ", c("Gene", "Value")) for(topV in seq_along(topGenes.values)){ ... colnames(complete.top) <-colnames
avoid 'copy-and-append' where a vector is grown over iterations; use *apply() or pre-allocate and fill, e.g., cbaf-automatedStatistics.R:338
for(gg in 1:length(sourceDataList)){ ... if(cs == 1){ temList$Top.Genes.of.Frequency.Percentage <- post.topGenes } else if(cs!=1){ temList$Top.Genes.of.Frequency.Percentage <- rbind(temList$Top.Genes.of.Frequency.Percentage, post.topGenes) }
might be
Top.Genes.of.Frequency.Percentage <- vector("list", length(sourceDataList)) for (gg in seq_along(sourceDataList)) { ... Top.Genes.of.Frequency.Percentage[[g]] <- post.topGenes } temList$Top.Genes.of.Frequency.Percentage <- do.call("rbind", Top.Genes.of.Frequency.Percentage)
I'm using Rstudio for programming. Unfortunately, there is no way to increase the distance between lines. As a result, the code looks hard to read. It looks like they are working to add this feature to Rstudio. The double line spacing was the only way I found to prevent confusion with code. I insist it stays this way for now. I promise to decrease the distance when Rstudio supports that feature.
All assign()
cases were removed. Database functions now directly store the data in the installed cbaf directory and availableData()
function uses excel output only.
All the branch are now logical.
at cbaf-availableData.R:206, one branch of the conditional returns a logical vector, the other a character vector. Use consistent data types (logical, in this case) to simplify subsequent code.
obtainOneStudy()
and cleanDatabase()
(The printed information is short).I also did three other changes:
A new cleanDatabase()
function that removes the obtained databases. (I had to add this as a result of storing the databbase in cbaf directory.)
obtainOneStudy()
and obtainMultipleStudies()
now check the database date. If it is more than 5 days old, they will remove it first.
News updated (It was mistakenly incomplete before).
*** I forgot to metion that a new installation section has been added to vignette.
Received a valid push; starting a build. Commits are:
fd7f590 - Critical bug fix
15 hours is a little bit too much time for package building. Isn't it?
The linux builder failed to complete; we @lshep will look into this.
Dear Package contributor,
This is the automated single package builder at bioconductor.org.
Your package has been built on Linux, Mac, and Windows.
On one or more platforms, the build results were: "skipped, ERROR". This may mean there is a problem with the package that you need to fix. Or it may mean that there is a problem with the build system itself.
Please see the following build report for more details:
http://bioconductor.org/spb_reports/cbaf_buildreport_20170913075244.html
Please ignore this first build report. It is an ERROR because of the run earlier that hung. I will manually kick off another build report for you and it should post momentarily; if it hangs again I will try and track down more information for you.
Dear Package contributor,
This is the automated single package builder at bioconductor.org.
Your package has been built on Linux, Mac, and Windows.
Congratulations! The package built without errors or warnings on all platforms.
Please see the following build report for more details:
http://bioconductor.org/spb_reports/cbaf_buildreport_20170913075823.html
@lshep Thank you very much.
Thank you for your updates; I have some additional comments before accepting the package.
I continue to see 1:
instead of seq_len()
or seq_along()
.
cbaf-automatedStatistics.R:490: for(fp in 1:geneNumber){
cbaf-automatedStatistics.R:678: for(empty in 1:newUnits){
cbaf-automatedStatistics.R:725: for(fr in 1:geneNumber){
cbaf-automatedStatistics.R:834: for(mv in 1:geneNumber){
cbaf-automatedStatistics.R:1020: for(empty in 1:newUnits){
cbaf-automatedStatistics.R:1067: for(mdv in 1:geneNumber){
cbaf-automatedStatistics.R:1254: for(empty in 1:newUnits){
cbaf-availableData.R:249: rownames(combined.list) <- 1:nrow(combined.list)
cbaf-availableData.R:268: rownames(combined.list.dataframe) <- 1:nrow(combined.list)
cbaf-heatmapOutput.R:525: heatmap.data <- heatmap.data[ordering[1:genelimit],]
cbaf-obtainMultipleStudies.R:714: for(eval in 1:ncol(this.segment)){
cbaf-obtainOneStudy.R:688: for(eval in 1:ncol(this.segment)){
prefer 'vectorized' code to iterations. For instance, at cbaf-automatedStatistics.R:490 use colSums()
, colMeans()
, etc. This will have significant performance consequences when the number of iterations is moderate or large.
lines like cbaf-availableData.R:227, with =="TRUE"
seem highly suspicious -- this looks like a logical vector treated as a character; why not treat it as logical?
cbaf-cleanDatabase.R:11 unlink
is vectorized, so unlink(full.path.of.correct.directories, recursvie=TRUE)
cbaf-obtainMultipleStudies.R:263 use additional arguments to system.file()
rather that paste()
, system.file("extdata", submissionName, package="cbaf")
cbaf-obtainMultipleStudies.R:423 allocate without iteration as rawList <- vector("list", length(geneList)); names(rawList) <- names(genesList)
and similar elsewhere.
Thank you for limiting width to 80 characters for long lines, instead of wrapping at the extreme left
automatedStatisticsProgressBar <- txtProgressBar(min = 0, max = total.number
, style = 3)
try alternative but commonly accepted strategies like starting the right-hand side on a new line
automatedStatisticsProgressBar <-
txtProgressBar(min = 0, max = total.number, style = 3)
or using the ()
in a way analogous to {}
automatedStatisticsProgressBar <- txtProgressBar(
min = 0, max = total.number, style = 3
)
the latter can be especially useful for function calls and data.frame construction
complete.top <-
data.frame(
topGene = topGene.name,
topValue = topGenes.values[topV],
stringsAsFactors = FALSE
)
in other cases it makes sense to un-nest the subsetting, as mentioned in the previous review, e.g., cbaf-obtainMultipleStudies.R:690
idx <- colnames(validationMatrix) %in% genesWithData[re]
colnames(validationMatrix)[idx] <- ...
Received a valid push; starting a build. Commits are:
ccf706e - Better formatting - Minor Improvements
Dear Package contributor,
This is the automated single package builder at bioconductor.org.
Your package has been built on Linux, Mac, and Windows.
Congratulations! The package built without errors or warnings on all platforms.
Please see the following build report for more details:
http://bioconductor.org/spb_reports/cbaf_buildreport_20170926153839.html
Thank you very much for your kind and informative comments. During the review process, I always felt you are my supervisor rather than a reviewer, for which I'm very grateful.
The following is the list of what I did:
seq_len()
and seq_along()
work in a similar manner and they can only create a sequence of numbers for a vector. But, working with sample code, proved that I was wrong; All of the code blocks that you specified, now use seq_len()
instead.I continue to see 1: instead of seq_len() or seq_along().
cbaf-automatedStatistics.R:490: for(fp in 1:geneNumber){ cbaf-automatedStatistics.R:678: for(empty in 1:newUnits){ cbaf-automatedStatistics.R:725: for(fr in 1:geneNumber){ cbaf-automatedStatistics.R:834: for(mv in 1:geneNumber){ cbaf-automatedStatistics.R:1020: for(empty in 1:newUnits){ cbaf-automatedStatistics.R:1067: for(mdv in 1:geneNumber){ cbaf-automatedStatistics.R:1254: for(empty in 1:newUnits){ cbaf-availableData.R:249: rownames(combined.list) <- 1:nrow(combined.list) cbaf-availableData.R:268: rownames(combined.list.dataframe) <- 1:nrow(combined.list) cbaf-heatmapOutput.R:525: heatmap.data <- heatmap.data[ordering[1:genelimit],] cbaf-obtainMultipleStudies.R:714: for(eval in 1:ncol(this.segment)){ cbaf-obtainOneStudy.R:688: for(eval in 1:ncol(this.segment)){
colSums()
and colMeans()
are faster, but I had to use mean()
because I wanted to manually assign values in some conditions: for example, when all numbers are under specific cutoff, of course, calculating the mean with zero input will yield NaN, but the actual data exists! I think there was a problem with subsetting as well that prevented me to do that. I have tried the function with 200 genes and 30 cancers. It takes around 10 seconds to finish. My problem is obaitnOneStudy()
and obtainMultipleStudies()
functions. They take much longer to finish because they need continues communications with the server. I hope parallel usage of for()
may decrease the elapsed time in future cbaf
releases.prefer 'vectorized' code to iterations. For instance, at cbaf-automatedStatistics.R:490 use
colSums()
,colMeans()
, etc. This will have significant performance consequences when the number of iterations is moderate or large.
sapply()
have the same type of output? Wouldn't 'implicit coercion' fix the problem?lines like cbaf-availableData.R:227, with
=="TRUE"
seem highly suspicious -- this looks like a logical vector treated as a character; why not treat it as logical?
cbaf-cleanDatabase.R:11
unlink
is vectorized, sounlink(full.path.of.correct.directories, recursvie=TRUE)
.
cbaf-obtainMultipleStudies.R:263 use additional arguments to
system.file()
rather thatpaste()
,system.file("extdata", submissionName, package="cbaf")
.
cbaf-obtainMultipleStudies.R:423 allocate without iteration as
rawList <- vector("list", length(geneList)); names(rawList) <- names(genesList)
and similar elsewhere.
Thank you for limiting width to 80 characters for long lines, instead of wrapping at the extreme left
automatedStatisticsProgressBar <- txtProgressBar(min = 0, max = total.number , style = 3)
try alternative but commonly accepted strategies like starting the right-hand side on a new line
automatedStatisticsProgressBar <- txtProgressBar(min = 0, max = total.number, style = 3)
or using the
()
in a way analogous to{}
.automatedStatisticsProgressBar <- txtProgressBar( min = 0, max = total.number, style = 3 )
the latter can be especially useful for function calls and data.frame construction
complete.top <- data.frame( topGene = topGene.name, topValue = topGenes.values[topV], stringsAsFactors = FALSE )
in other cases it makes sense to un-nest the subsetting, as mentioned in the previous review, e.g., cbaf-obtainMultipleStudies.R:690
idx <- colnames(validationMatrix) %in% genesWithData[re] colnames(validationMatrix)[idx] <- ...
Your package has been accepted. It will be added to the Bioconductor Git repository and nightly builds. Additional information will be sent to the maintainer email address in the next several days.
Thank you for contributing to Bioconductor!
The master branch of your GitHub repository has been added to Bioconductor's git repository.
To use the git.bioconductor.org repository, we need an 'ssh' key to associate with your github user name. If your GitHub account already has ssh public keys (https://github.com/armanshahrisa.keys is not empty), then no further steps are required. Otherwise, do the following:
See further instructions at
https://bioconductor.org/developers/how-to/git/
for working with this repository. See especially
https://bioconductor.org/developers/how-to/git/new-package-workflow/ https://bioconductor.org/developers/how-to/git/sync-existing-repositories/
to keep your GitHub and Bioconductor repositories in sync.
Your package will be included in the next nigthly 'devel' build (check-out from git at about 6 pm Eastern; build completion around 2pm Eastern the next day) at
https://bioconductor.org/checkResults/
(Builds sometimes fail, so ensure that the date stamps on the main landing page are consistent with the addition of your package). Once the package builds successfully, you package will be available for download in the 'Devel' version of Bioconductor using biocLite(\"cbaf\")
. The package 'landing page' will be created at
https://bioconductor.org/packages/cbaf
If you have any questions, please contact the bioc-devel mailing list (https://stat.ethz.ch/mailman/listinfo/bioc-devel); this issue will not be monitored further.
Update the following URL to point to the GitHub repository of the package you wish to submit to Bioconductor
Confirm the following by editing each check box to '[x]'
[x] I understand that by submitting my package to Bioconductor, the package source and all review commentary are visible to the general public.
[x] I have read the Bioconductor Package Submission instructions. My package is consistent with the Bioconductor Package Guidelines.
[x] I understand that a minimum requirement for package acceptance is to pass R CMD check and R CMD BiocCheck with no ERROR or WARNINGS. Passing these checks does not result in automatic acceptance. The package will then undergo a formal review and recommendations for acceptance regarding other Bioconductor standards will be addressed.
[x] My package addresses statistical or bioinformatic issues related to the analysis and comprehension of high throughput genomic data.
[x] I am committed to the long-term maintenance of my package. This includes monitoring the support site for issues that users may have, subscribing to the bioc-devel mailing list to stay aware of developments in the Bioconductor community, responding promptly to requests for updates from the Core team in response to changes in R or underlying software.
I am familiar with the essential aspects of Bioconductor software management, including:
For help with submitting your package, please subscribe and post questions to the bioc-devel mailing list.