AMARETTO new package submission

ogevaert commented 5 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/gevaertlab/AMARETTO

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 Git (optionally via GitHub).

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

bioc-issue-bot commented 5 years ago

Hi @ogevaert

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: AMARETTO
Type: Package
Title: Regulatory network inference and driver gene evaluation using integrative multi-omics analysis and penalized regression.
Version: 0.99.1
Date: 2016-06-06
Author: Jayendra Shinde, Celine Everaert, Shaimaa Bakr, Mohsen Nabian
    Nathalie Pochet and Olivier Gevaert
Maintainer: Olivier Gevaert <olivier.gevaert@gmail.com>
Depends: R (>= 3.5), RCurl,limma, foreach, parallel, doParallel,
        glmnet (>= 2.0.16), matrixStats, RColorBrewer, impute, Matrix, BiocStyle,
        stringr, ComplexHeatmap, circlize, R.utils, randomcoloR,
        curatedTCGAData, tidyverse, callr, Rcpp, DT, htmltools,
    reshape2, rmarkdown
Description: Module network discovery using gene expression, CNV, and methylation data.
License: MIT + file LICENSE
LazyLoad: yes
Encoding: UTF-8
biocViews:
        StatisticalMethod,DifferentialMethylation,GeneRegulation,GeneExpression,MethylationArray,Transcription,Preprocessing,BatchEffect,DataImport,mRNAMicroarray,MicroRNAArray,Regression,Clustering,RNASeq,CopyNumberVariation,Sequencing,Microarray,Normalization,Network,Bayesian,ExonArray,OneChannel,TwoChannel,ProprietaryPlatforms,AlternativeSplicing,DifferentialExpression,DifferentialSplicing,GeneSetEnrichment,MultipleComparison,QualityControl,TimeCourse
Suggests: 
    testthat,
    MASS,
    knitr,
    rmarkdown
NeedsCompilation: no
Packaged: 2016-06-08 11:54:24 UTC
Imports: 
    callr (>= 3.0.0.9001),
    Rcpp
Roxygen: list(markdown = TRUE)
RoxygenNote: 6.1.0
LinkingTo: 
    Rcpp
VignetteBuilder: knitr
Remotes: 
    r-lib/callr

Add SSH keys to your GitHub account. SSH keys will are used to control access to accepted Bioconductor packages. See these instructions to add SSH keys to your GitHub account.

bioc-issue-bot commented 5 years ago

A reviewer has been assigned to your package. Learn what to expect during the review process.

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 5 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 build report for more details.

bioc-issue-bot commented 5 years ago

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

98caee0 Update DESCRIPTION

bioc-issue-bot commented 5 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, 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 build report for more details.

bioc-issue-bot commented 5 years ago

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

ca2c5b5 Bioconductor submission

bioc-issue-bot commented 5 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, 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 build report for more details.

bioc-issue-bot commented 5 years ago

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

2b659e9 Bioconductor submission

ogevaert commented 5 years ago

@jayendrashinde91

bioc-issue-bot commented 5 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, 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 build report for more details.

bioc-issue-bot commented 5 years ago

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

96e9c65 Bioconductor submission

bioc-issue-bot commented 5 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, 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 build report for more details.

bioc-issue-bot commented 5 years ago

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

af20c08 Bioconductor submission

bioc-issue-bot commented 5 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, 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 build report for more details.

bioc-issue-bot commented 5 years ago

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

87ee2c3 Bioconductor submission

bioc-issue-bot commented 5 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, 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 build report for more details.

bioc-issue-bot commented 5 years ago

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

644002f Bioconductor submission

bioc-issue-bot commented 5 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 build report for more details.

bioc-issue-bot commented 5 years ago

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

4a57cd7 Bioconductor submission

bioc-issue-bot commented 5 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, TIMEOUT, 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 build report for more details.

bioc-issue-bot commented 5 years ago

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

5688954 Bioconductor submission

bioc-issue-bot commented 5 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 build report for more details.

bioc-issue-bot commented 5 years ago

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

b44f2ab Bioconductor submission

bioc-issue-bot commented 5 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 build report for more details.

bioc-issue-bot commented 5 years ago

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

5fef8f2 Bioconductor submission

bioc-issue-bot commented 5 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 build report for more details.

bioc-issue-bot commented 5 years ago

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

6d1283f Bioconductor submission

bioc-issue-bot commented 5 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 build report for more details.

bioc-issue-bot commented 5 years ago

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

85b86b0 Bioconductor submission

bioc-issue-bot commented 5 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, 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 build report for more details.

bioc-issue-bot commented 5 years ago

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

6290b76 Bioconductor submission

bioc-issue-bot commented 5 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 build report for more details.

ogevaert commented 5 years ago

@mtmorgan Hi Martin, just a note, we have been able to resolve all warnings and errors. Let us know if we need to do anything else to have the package reviewed.

mtmorgan commented 5 years ago

DESCRIPTION

Consider a more extensive Description: field, like a paper abstract.
The License: field says that the package is licensed under MIT but the LICENSE file is Apache. Please clarify.

vignettes

please remove vignette 'build' products, i.e., the pdf file, from the vignettes directory; these are created when the package is installed.
the vignette needs to contain evaluated R code chunks, rather than static code. Most code in the vignette should be evaluated, with the exception of package installation instructions.
Use TargetDirectory <- tempfile() for temporary file locations.
usually only a single .gitignore file (in the top-level directory) is required.

R: the following comments are about specific locations in your code, but the issues come in a number of places.

Consider using BiocFileCache so that both your user and the build system only download large files once.
e.g., amaretto_download.R:21 use message() rather than cat() when providing informative messages to users. Note that message() pastes its arguments together, so that cat(paste0("foo ", bar)) becomes message("foo ", bar).
e.g., amaretto_download.R:42 this command is only executed if there are no prior errors, otherwise the user is surprised to find their directory changed 'behind their backs'. At the start of the function, ensure that the original directory is restored no matter what happens by using
```
ori.dir <- getwd()
on.exit(setwd(ori.dir)
```
amaretto_download.R:59 I'm not an expert on TCGA, but am concerned that your efforts duplicate those available in other Bioconductor packages; maybe @LiNk-NY or @lwaldron can comment further.
amaretto_download.R:99 return(cat(...)) is confusing use
```
cat(...)
return()
```
amaretto_download.R:104 use httr instead of getURL() and other approaches to internet access. For instance, stop_for_status() can be used to test for failed access, rather than trying to scrape (amaretto_download.R:143) output values. Likewise facilities for checking status, write_disk(), etc.
amaretto_download.R:147 stop(paste0(...)) can usually be written stop(...). On the other hand amaretto_visualize.R:43 needs to include spaces "... Module ", ModuleNr, " since..."
amaretto_functions.R:185 use seq_len(AMARETTOresults$NrModules) to avoid surprise when AMARETTOresults$NrModules == 0
amaretto_functions.R:209 use 'pre-allocate and fill' or simply lappy() and friends rather than inefficient copy and append; see robust and efficient code.
amaretto_functions.R:217 use vectorized (one call to an R function) rather than iterative (one call per element) calculations, e.g., colMeans() rather than apply(., 2, mean). See also functionality in the matrixStats package

amaretto_htmlreport.R:88 this and other code in this file is basically unreadable! break at 'logical' locations rather than at fixed width, e.g.,

dt_regulators <- 
  datatable(
    rownames_to_column(as.data.frame(ModuleRegulators), "RegulatorIDs") %>% 
    dplyr::rename(Weights = "ModuleRegulators") %>% 
    mutate(
      RegulatorIDs = paste0(
        "<a href=\"https://www.genecards.org/cgi-bin/carddisp.pl?gene=", RegulatorIDs, "\">", 
        RegulatorIDs, 
        "</a>"
      )
    ), 
    class = "display", 
    extensions = "Buttons", 
    rownames = FALSE, 
    options = list(columnDefs = list(list(width = "200px", targets = "_all")), 
    pageLength = 10, 
    dom = "Bfrtip", 
    buttons = c("csv", "excel", "pdf")), 
    escape = "Weights"
 ) %>%  ...

consider other approaches, too, e.g.,, defining base_url = ... outside the complicated experssion or using whisker. Many other examples of unusual rather than (mathematically) logical line breaks generally make your code very difficult to read.

amaretto_preprocess.R: 674 Another (of many!) examples where a simple lapply would be both easier to read and more efficient.

man

pay attention to line widths, especially in examples, where many environments present man pages formatted to a width of 80 characters.
most man pages should have runnable code (not just loading a data set like AMARETTO_EvaluateTestSet.Rd!)

other

please 'clean up' the master branch of the repository to include only files necessary for your package, e.g., removing AMARETTO-manual.pdf and files/.

jayendrashinde91 commented 5 years ago

@celineeveraert @monabiyan

lwaldron commented 5 years ago

The TCGA data access does seem extremely complicated, getting RNAseq2GeneNorm from curatedTCGAData and GISTIC with a long get_firehoseData() function copied from MethylMix. The following probably does what you need:

library(curatedTCGAData)
library(TCGAutils)
mae.orig <- curatedTCGAData("ACC", assays=c("GISTIC_AllByGene", "RNASeq2GeneNorm"), dry.run = FALSE)
# Separate normal tissues etc; this might be followed by removal of 
# unwanted sample types or selection of wanted sample types
mae <- splitAssays(mae.orig)
# Merge any remaining replicates
mae <- mergeReplicates(mae)
# Only if you want to align specimens / columns of each assay
mae <- intersectColumns(mae)
# Only if you want to align genes / rows of each assay
mae <- intersectRows(mae)
listofmatrices <- assays(mae) #can write to disk or do whatever you want then

A few other thoughts in addition to Martin's:

amaretto_functions.R:31: It seems unconventional and maybe dangerous to supersede the assays generic (which has methods for MultiAssayExperiment and SummarizedExperiment) with a data object
amaretto_functions.R:34: Why serialize then save curatedTCGAData results? It duplicate the caching procedure that ExperimentHub is already doing automatically, except with extra steps and disk usage, and copying files from the ExperimentHub cache to another directory every time the function is run
- amaretto_functions.R:251: Creates an output device within a function to create a pdf, so there's no user control over figure size, fonts, etc. Maybe this needs to be done because of many pages of output, but can you at least use ... to pass arguments to pdf() to allow users to modify the output device? Question: does this create huge pdf files due to large cluster dendrograms and size of heatmap grid?
amaretto_functions.R:271: It isn't necessary to include paste0 inside file.path(), which already performs a paste0 on its arguments.

Vignette: Note that >BiocManager::install("gevaertlab/AMARETTO") installs from GitHub, not Bioconductor

LiNk-NY commented 5 years ago

I agree with Levi @lwaldron and Martin @mtmorgan.

A couple of additional notes:

I would avoid saving PDFs for the user (unless explicitly requested by the user).
There are functions that identify samples in assays such as sampleTables in TCGAutils in conjunction with data("sampleTypes") which would save you some code in TCGA_GENERIC_GetSampleGroups. You can also look into TCGAsampleSelect.

Best, Marcel

nathaliepochet commented 5 years ago

Thanks so much @lwaldron, @mtmorgan and @LiNk-NY for your suggestions! We'll be making improvements according to your suggestions!

Kind regards, Nathalie & Olivier

jayendrashinde91 commented 5 years ago

Thank you @lwaldron for this interesting suggestion. We have tested the packages curatedTCGAData and TCGAutils to download data. Indeed the firehose approach is a little complicated, but GISTIC data from curatedTCGAData does not match in the number of samples to the GISTIC data from Broad Firehose for few cancer types. This was the motivation to stick to get_firehoseData(). Could you kindly recommend a way to circumvent this issue in implementing download with curatedTCGAData()?

Best regards, Jay & Olivier

lwaldron commented 5 years ago

Which GISTIC profiles are missing (or extra)? I'm surprised, and would like to investigate...

Il lun 18 feb 2019, 8:31 PM Jayendra Shinde notifications@github.com ha scritto:

Thank you @lwaldron https://github.com/lwaldron for this interesting suggestion. We have tested the packages curatedTCGAData and TCGAutils to download data. Indeed the firehose approach is a little complicated, but GISTIC data from curatedTCGAData does not match in the number of samples to the GISTIC data from Broad Firehose for few cancer types. This was the motivation to stick to get_firehoseData(). Could you kindly recommend a way to circumvent this issue in implementing download with curatedTCGAData()?

Best regards, Jay & Olivier

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/Bioconductor/Contributions/issues/1001#issuecomment-464942706, or mute the thread https://github.com/notifications/unsubscribe-auth/AAnDxjm0FLvpYZT8pv64hyF0cymldN5Yks5vO1PagaJpZM4arUIW .

bioc-issue-bot commented 5 years ago

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

45dd767 bioconductor review updates

bioc-issue-bot commented 5 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, TIMEOUT, 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 build report for more details.

mtmorgan commented 5 years ago

I'm not exactly sure where this issue stands? Is it ready for continue review?

ogevaert commented 5 years ago

We are working with Vincent to fix a number of other issues as well. So we will be ready and respond to your review in the next few days.

bioc-issue-bot commented 5 years ago

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

a005685 version bump

bioc-issue-bot commented 5 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 build report for more details.

bioc-issue-bot commented 5 years ago

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

86b24df use_build_ignore

bioc-issue-bot commented 5 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, 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 build report for more details.

bioc-issue-bot commented 5 years ago

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

a59a1b0 R version bump for BioC

bioc-issue-bot commented 5 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 build report for more details.

Bioconductor / Contributions

AMARETTO new package submission #1001