federicogiorgi commented 7 years ago

Update the following URL to point to the GitHub repository of the package you wish to submit to Bioconductor

Repository: https://github.com/federicogiorgi/vulcan

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:

[x] The 'devel' branch for new packages and features.
[x] The stable 'release' branch, made available every six months, for bug fixes.
[x] Bioconductor version control using Subversion (optionally via GitHub).

For help with submitting your package, please subscribe and post questions to the bioc-devel mailing list.

bioc-issue-bot commented 7 years ago

Hi @federicogiorgi

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: vulcan
Type: Package
Title: VirtUaL ChIP-Seq data Analysis using Networks
Version: 0.99.0
Author: Federico M. Giorgi
Maintainer: Federico M. Giorgi <federico.giorgi@gmail.com>
Description: This package interrogates regulatory networks to infer cofactors
    significantly enriched in a differential binding signature coming from
    ChIP-Seq data
License: LGPL-3
LazyData: TRUE
biocViews: Genome, Homo_sapiens_Data
NeedsCompilation: no
Suggests: vulcandata
Depends: R (>= 3.3), IRanges, DiffBind, viper, ChIPpeakAnno, gplots,
    TxDb.Hsapiens.UCSC.hg19.knownGene, DESeq, zoo, wordcloud, csaw
Encoding: UTF-8
RoxygenNote: 6.0.1

federicogiorgi commented 7 years ago

AdditionalPackage: https://github.com/federicogiorgi/vulcandata

mtmorgan commented 7 years ago

The vignette is not adequate; let me know (comment on this issue) when you have a vignette describing overall use of the package, including integration with other Bioconductor packages. Remember that Bioconductor vignettes include code chunks that actually exercise your code.

Also, the biocViews terms are confusing -- this looks like a 'software' package (i.e., substantial R code) but the views include 'Annotation' package terms (Homo_sapiens_Data); software packages should choose terms from the software hierarchy.

federicogiorgi commented 7 years ago

Dear Martin and Community. I've updated the biocViews and vignette for your inspection: https://github.com/federicogiorgi/vulcan https://github.com/federicogiorgi/vulcandata

bioc-issue-bot commented 7 years ago

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.

bioc-issue-bot commented 7 years ago

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/vulcan_buildreport_20170608052905.html

mtmorgan commented 7 years ago

@lshep can you help with instructions for solving the build issue -- vulcandata data package as a dependency here; I can never keep straight what the Depends / Imports / Suggests relationship between data and software packages is supposed to be, and I don't know which package is supposed to be added to the tracker first. And in the current case I really don't know how to get the SPB back on track!

lshep commented 7 years ago

@mtmorgan The suggests and depends are correct. The vulcan package suggests the vulcandata data package and the data package depends on the software package. Its slightly tricky in this case as we need both packages to be installed to pass - now that vulcan has been installed @federicogiorgi please make sure the webhook is also set up on the vulcandata repository and do a version bump on vulcandata. This should build and install vulcandata. Once that package has a clean install, build, check , a version bump on the vulcan software package would then find vulcandata and be able to build

bioc-issue-bot commented 7 years ago

Received a valid push; starting a build. Commits are:

6ddc502 Update

federicogiorgi commented 7 years ago

@ishep @mtmorgan I set up the webhooks and did a version bump. Thanks a lot for the help :-)

bioc-issue-bot commented 7 years ago

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/vulcan_buildreport_20170608082717.html

lshep commented 7 years ago

@federicogiorgi Please set the webhook and Bump the version on vulcandata not vulcan

federicogiorgi commented 7 years ago

@ishep I did bump the version and set up the webhook on vulcandata, but somehow my push doesn't trigger a build of vulcandata

federicogiorgi commented 7 years ago

AdditionalPackage: https://github.com/federicogiorgi/vulcandata

bioc-issue-bot commented 7 years ago

Hi @federicogiorgi,

Starting build on additional package https://github.com/federicogiorgi/vulcandata.

IMPORTANT: Please read the instructions for setting up a push hook on your repository, or further changes to your additional package repository will NOT trigger a new build.

The DESCRIPTION file of this additional package is:

Package: vulcandata
Type: Package
Title: VirtUaL ChIP-Seq data Analysis using Networks, dummy dataset
Version: 0.99.3
Author: Federico M. Giorgi
Maintainer: Federico M. Giorgi <federico.giorgi@gmail.com>
Description: This package provides a dummy regulatory network and ChIP-Seq
dataset for running examples in the vulcan package
License: LGPL-3
LazyData: TRUE
biocViews: ExperimentData, Homo_sapiens_Data, CancerData, ChIPSeqData
NeedsCompilation: no
Depends: vulcan
Encoding: UTF-8
LazyData: true
RoxygenNote: 6.0.1

federicogiorgi commented 7 years ago

@ishep it seems I had to rewrite the "AdditionalPackage" thing :-) it seems to be working now.

lshep commented 7 years ago

Thank you. I will try and keep an eye on this too but if you find it is not building the package you version bumped please let me know.

bioc-issue-bot commented 7 years ago

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/vulcandata_buildreport_20170608084153.html

bioc-issue-bot commented 7 years ago

Received a valid push; starting a build. Commits are:

fa2db30 Update

bioc-issue-bot commented 7 years ago

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/vulcan_buildreport_20170608085026.html

bioc-issue-bot commented 7 years ago

Received a valid push; starting a build. Commits are:

6e986db Update

bioc-issue-bot commented 7 years ago

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/vulcan_buildreport_20170608090431.html

bioc-issue-bot commented 7 years ago

Received a valid push; starting a build. Commits are:

b7a5a32 Update

bioc-issue-bot commented 7 years ago

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: "WARNINGS, 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/vulcan_buildreport_20170608093620.html

bioc-issue-bot commented 7 years ago

Received a valid push; starting a build. Commits are:

650fa50 Update

bioc-issue-bot commented 7 years ago

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: "WARNINGS". 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/vulcan_buildreport_20170608101523.html

bioc-issue-bot commented 7 years ago

Received a valid push; starting a build. Commits are:

8f31e5d Update

bioc-issue-bot commented 7 years ago

Received a valid push; starting a build. Commits are:

99586a7 Update

federicogiorgi commented 7 years ago

For some reason, I've stopped receiving build reports since yesterday. Is there a problem?

lshep commented 7 years ago

we are looking into this and should have it fixed soon. sorry for the inconvenience.

bioc-issue-bot commented 7 years ago

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: "TIMEOUT". 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/vulcan_buildreport_20170613074127.html

nturaga commented 7 years ago

@federicogiorgi Please remove all the files which are not essential to the package by using a .gitignore file. Once that is done, please message when you are ready for a review.

bioc-issue-bot commented 7 years ago

Received a valid push; starting a build. Commits are:

b9a649a Update

federicogiorgi commented 7 years ago

I did add not essential files to the .gitignore list, and I am ready for a review :-)

bioc-issue-bot commented 7 years ago

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/vulcan_buildreport_20170614064835.html

bioc-issue-bot commented 7 years ago

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/vulcan_buildreport_20170626084748.html

nturaga commented 7 years ago

vulcan Review

Build

R-release CMD build vulcan: Delete Depends section in vulcandata. It does NOT depend on vulcan.

R-release CMD INSTALL vulcan : ok.

DESCRIPTION

Not enough in the Description section, it needs to be similar to the abstract of at paper. Shortening the overview in the vignette would be a good way to go about this.

NAMESPACE

Recreate with roxygen2, and do not edit by hand.

man

Some more work could help,
For example, 'vulcan.Rd : Value', more detail in the return value and also the description needs to be informative about method being employed.

vignette

Vignette is very limited and needs to be more of a walkthrough explaining the data inputs and outputs.
Mainly the vulcan pipeline doesn't explain "Why" the pipeline works in those steps.

R

Use formatR to follow the same style. (https://cran.r-project.org/web/packages/formatR)
In gsea.R: line 184, and 190.

184, Avoid using 1:np use seq_len(np). See (https://www.bioconductor.org/developers/how-to/efficient-code/)

190, consider the case when length of inSet is 0.
Delete code which is commented out , gsea.R : line 269 - 274. And similar in vulcan.R and other R scripts.
The code for each method (topvar, lowvar, strongest, etc) is repeated, and can be written as a function and reused here. Write a function which does that , and reuse for both rawcount matrix and rpkm matrix.

     if(method=="topvar"){
         for(i in seq_along(genes)){
             gene<-genes[i]
             if(peakspergene[gene]>1){
                 subanno<-dfanno[dfanno$feature==gene,]
                 vars<-apply(subanno[,allsamples],1,var)
                 top<-which.max(vars)
                 rawcounts[gene,allsamples]<-as.numeric(subanno[top,allsamples])
             }
         }
     }

Rearrange the code so that the for loop is only over possible genes

    idx0 <- seq_along(genes)
    idx <- idx0[peakspergene[idx0] > 1]
    for (gene in genes[idx]) {
        ...
    }

The identification of subanno is quadratic -- a scan of dfanno$feature for each gene. Also, the entire data frame dfanno is subset, but only the column allsamples is needed. Instead, do this in linear time and for just the data that is required.

subanno <- split(dfanno$allsamples, dfanno$feature)[gene]
vars <- sapply(subanno, var)

This code is not quite right but hopefully gives the idea -- the explicit loop over genes does not seem necessary.

l. 408 is 'copy-and-append', but should be 'pre-allocate and fill' or a simple lapply; see the 'robust code' link above.
Remove .Rhistory from your package.
Fix all the NOTES in the R CMD Check part of the build report.

vulcandata review

The experiment data packages need to have a vignette as well.
The vignette could describe how the data is derived and where it comes from, and how it could be used.

federicogiorgi commented 7 years ago

Thank you! I will implement the changes as soon as possible.

federicogiorgi commented 7 years ago

@nturaga I implemented your suggestions, thank you for making them, they were bright and enlightening, especially those on optimizing the loops. However, I kept the pipelines for RPKMs and Raw Counts separated, as they may be different in the future due to the different nature of the input data. I will upload the updated package shortly.

bioc-issue-bot commented 7 years ago

Received a valid push; starting a build. Commits are:

aa1652b Update

bioc-issue-bot commented 7 years ago

Received a valid push; starting a build. Commits are:

ae3a83c Update

bioc-issue-bot commented 7 years ago

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/vulcandata_buildreport_20170714104614.html

bioc-issue-bot commented 7 years ago

Received a valid push; starting a build. Commits are:

6b7b52a Update

bioc-issue-bot commented 7 years ago

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/vulcandata_buildreport_20170714105057.html

bioc-issue-bot commented 7 years ago

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/vulcan_buildreport_20170714110436.html

nturaga commented 7 years ago

hi @federicogiorgi i'll take a look at the changes and reply soon.

nturaga commented 7 years ago

Build/install ok.
man pages not changed.
l:159 in gsea.R, you do not need this line. You overwrite it in the very next line.

Please fix these instances as well, from 1:ncol/length() to seq_len. Please fix all instances of this "1:". Account for 0.

~/D/b/v/R ❯❯❯ grep "1:length" *                                              master ◼
generic.R:        for (i in 1:length(x)) {
gsea.R:            null_inSet <- inSet[sample(1:length(inSet))]
rea.R:    for (i in 1:length(wgroups)) {
vulcan.R:    for (i in 1:length(listcounts)) {
vulcan.R:    for (i in 1:length(listcounts)) {
vulcan.R:    names(dfanno) <- 1:length(dfanno)
vulcan.R:    names(dfanno) <- 1:length(dfanno)
vulcan.R:    for (i in 1:length(samples)) {
~/D/b/v/R ❯❯❯ grep "1:ncol" *                                                master ◼
vulcan.R:    for (j in 1:ncol(rawcounts)) {
vulcan.R:    for (j in 1:ncol(rpkms)) {

Fix NOTES:

* Checking formatting of DESCRIPTION, NAMESPACE, man pages, R source,
  and vignette source...
* NOTE: Consider shorter lines; 84 lines (3%) are > 80 characters
  long.
First 6 lines:
  R/generic.R:167 val2col <- function(z, col1 = "navy", col2 = "white", c...
  R/generic.R:319 textplot2 <- function(x, y, words, cex = 1, pch = 16, p...
  R/generic.R:339     text(lay[, 1] + 0.5 * lay[, 3], lay[, 2] + 0.5 * la...
  R/generic.R:370         legend("topright", paste0("fragment-length = ",...
  R/gsea.R:24 gsea <- function(reflist, set, method = c("permutation", "p...
  R/gsea.R:121             newp.value <- ppareto(abs(es), threshold = q95...
* NOTE: Consider multiples of 4 spaces for line indents, 11
  lines(0%) are not.
First 6 lines:
  R/gsea.R:287                   ytop = 10, col = colBarcode, border = NA)
  R/gsea.R:293                   srt = 90, offset = 0, pos = 4, font = 2)
  R/gsea.R:324                   3), sep = "")), bg = "white")
  R/rea.R:43              matrices of identical size")
  man/gsea.Rd:8   gsea_null = NULL)
  man/plot_gsea.Rd:8   colBarcode = "black", title = "Running Enrichment ...
See http://bioconductor.org/developers/how-to/coding-style/

These sets of repeated code can be made a function, and reused.

     # Method closest: when multiple peaks are found, keep only the closest to the TSS
 # as the representative one
 if (method == "closest") {
     for (gene in genesmore) {
         subanno <- dfanno[dfanno$feature == gene, ]
         closest <- which.min(subanno$distanceToStart)
         rawcounts[gene, allsamples] <- as.numeric(subanno[closest, allsamples])
     }
 }

 # Method farthest: when multiple peaks are found, keep only the closest to the
 # TSS as the representative one
 if (method == "farthest") {
     for (gene in genesmore) {
         subanno <- dfanno[dfanno$feature == gene, ]
         farthest <- which.max(subanno$distanceToStart)
         rawcounts[gene, allsamples] <- as.numeric(subanno[farthest, allsamples])
     }
 }

for example:

dist_calc <- function(method, dfanno, rawcounts, genesmore) {
    my_func <- switch (method,
        farthest = which.max,
        closest = which.min,
        ## more types of methods
        ## custom_function = function(x) ...
        ## default:
        stop("unsupported method ", method)
     )
    for (gene in genesmore) {
      subanno <- dfanno[dfanno$feature == gene, ]
      result <- my_func(subanno$distanceToStart)
      rawcounts[gene, allsamples] <- as.numeric(subanno[result, allsamples])
  }

Please fix all these issues.

bioc-issue-bot commented 7 years ago

Received a valid push; starting a build. Commits are:

a0bbcb5 Update

bioc-issue-bot commented 7 years ago

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/vulcan_buildreport_20170717134132.html

bioc-issue-bot commented 7 years ago

Received a valid push; starting a build. Commits are:

7259464 Update

Bioconductor / Contributions

VULCAN: VirtUaL ChIP-Seq data Analysis using Networks #379

vulcan Review

Build

DESCRIPTION

NAMESPACE

man

vignette

R

vulcandata review