gmelloni commented 6 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/gmelloni/PrecisionTrialDrawer

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 6 years ago

Hi @gmelloni

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: PrecisionTrialDrawer
Type: Package
Title: A Tool to Analyze and Design NGS Based Custom Gene Panels
Version: 0.99.0
Date: 2018-10-01
Author: Giorgio Melloni , Alessandro Guida
Maintainer: Giorgio Melloni <melloni.giorgio@gmail.com>
Description: We developed a tool to analyze list of genes and
    their alterations in order to create a custom panel for clinical genomics
    studies. It represents a resource to design basket or umbrella clinical trials and 
    provides a suite of methods to simulate an in silico prescription from a gene or 
    drug prospective. Finally, it is possible to design the genomic ranges 
    that will be sequenced in ready-to-be-submitted bed format.
License: GPL-3
Depends:
    R (>= 3.5)
Imports:
    graphics,
    grDevices,
    stats,
    utils,
    methods,
    cgdsr,
    parallel,
    stringr,
    reshape2,
    data.table,
    RColorBrewer,
    BiocParallel,
    magrittr,
    biomaRt,
    XML,
    httr,
    jsonlite,
    ggplot2,
    ggrepel,
    grid,
    S4Vectors,
    IRanges,
    GenomicRanges,
    LowMACAAnnotation,
    googleVis,
    shiny,
    shinyBS,
    DT,
    brglm
Suggests:
    BiocStyle,
    knitr,
    rmarkdown,
    dplyr,
    testthat
VignetteBuilder: knitr
biocViews: SomaticMutation, WholeGenome, Sequencing, DataImport
RoxygenNote: 5.0.1

bioc-issue-bot commented 6 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 6 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.

nturaga commented 6 years ago

Please correct all the ERROR and follow up with any NOTE's you may have in the build report. The build report should be as clean as possible.

Some NOTE's which may come up will include coding-style and efficiency issues, and will be pointed out by the build report. Please follow up with these as well.

Best,

Nitesh

On Tue, Oct 2, 2018 at 11:54 AM bioc-issue-bot notifications@github.com wrote:

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 http://bioconductor.org/spb_reports/PrecisionTrialDrawer_buildreport_20181002115336.html for more details.

— You are receiving this because you were assigned. Reply to this email directly, view it on GitHub https://github.com/Bioconductor/Contributions/issues/891#issuecomment-426327788, or mute the thread https://github.com/notifications/unsubscribe-auth/ACnoS-5J-4aLdtODsh-NobYyb4LvFMhbks5ug4wqgaJpZM4XCva- .

gmelloni commented 6 years ago

I think I solved the ERROR. How do I get a new build attempt?

nturaga commented 6 years ago

You need to update the "Version" in the DESCRIPTION file.

Take a look at http://bioconductor.org/developers/package-submission/#whattoexpect.

To trigger a new build, include a version bump in your commit, e.g., from Version: 0.99.0 to Version: 0.99.1. Pre-release versions utilize the 0.99.z format. When accepted and released, your package’s version number will be automatically incremented to 1.0.0.

Also, other helpful package guidelines, http://bioconductor.org/developers/package-guidelines/

bioc-issue-bot commented 6 years ago

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

6267bd7 Version BUMP

bioc-issue-bot commented 6 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 6 years ago

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

9ee2d8d some cleanup of tests cb0920c add GeneExpression in biocViews 64934ff faster examples 34e945f minor code style improvements b45ea1b add NEWS and CITATION files

bioc-issue-bot commented 6 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 6 years ago

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

566132c tabs to 4 spaces b49a87b refactoring of sapply to vapply and 1: to seq_len 51f3b34 bump version 0.99.3

bioc-issue-bot commented 6 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: "ABNORMAL". 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 6 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 6 years ago

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

c1d18ac add smaller datasets for examples c043497 faster and shorter examples 1d8f296 better error handling for empty data 5b2ae96 tiny typo e8249ab bug fix e79b642 some code cleanup 4c21ce7 bump 0.99.4

bioc-issue-bot commented 6 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.

gmelloni commented 6 years ago

@nturaga I have no idea how I can go below 5 minutes on the Windows server. I cut the examples to the bare minimum but the package is quite complex.

bioc-issue-bot commented 6 years ago

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

5daf538 reference to cgdsr reinserted dd57f76 bump 0.99.5

bioc-issue-bot commented 6 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 6 years ago

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

5e6de99 reduced example dataset

bioc-issue-bot commented 6 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 6 years ago

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

ec3805f add html files in Rbuildignore 116251b bug fix in aa to genomic b4a7446 running time reduced by 100 secs ecfa16d faster tests c8ca63e example data update c96be62 manual for new cpDesign example data 63a1ce4 removed test for cancer panel constructor 2627c2b update to version 0.99.7 33ebc7d bump version 0.99.7

bioc-issue-bot commented 6 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 6 years ago

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

db40a29 rebump 0.99.8

bioc-issue-bot commented 6 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 6 years ago

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

4c59ae6 faster examples and no tests 8c05ad7 bump version 0.99.9

bioc-issue-bot commented 6 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.

gmelloni commented 6 years ago

@nturaga It seems that the time the package takes to build and check depends heavily on the connection to biomart and cgdsr. It took 5 minutes and 4 seconds on the Windows server on version 0.99.8. After cutting the examples even more and removed all the tests now all the servers are over 5 minutes on version 0.99.9.

I don't know how to bump another version at this point

nturaga commented 6 years ago

You can do 0.99.10.

bioc-issue-bot commented 6 years ago

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

be8a4c8 bump 0.99.10

bioc-issue-bot commented 6 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.

gmelloni commented 6 years ago

@nturaga no more warnings. what's next?

nturaga commented 6 years ago

I'll review your package.

On Mon, Oct 15, 2018, 1:43 PM Giorgio Melloni notifications@github.com wrote:

@nturaga https://github.com/nturaga no more warnings. what's next?

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

-- Nitesh Turaga

nturaga commented 6 years ago

Please bump your version

gmelloni commented 6 years ago

Latest version is 0.99.10. I am not sure what action I should take, sorry

nturaga commented 6 years ago

Bump the version to 0.99.11. There is some issue with the previous version bump and your package has been flagged. It doesn't need a change in the code or anything, just a bump in the version number.

bioc-issue-bot commented 6 years ago

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

6361ad7 no changes

bioc-issue-bot commented 6 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.

gmelloni commented 6 years ago

@nturaga done :)

nturaga commented 5 years ago

PrecisionTrialDrawer review

DESCRIPTION

ok

NAMESPACE

ok

data

ok

inst

The NEWS file doesn't have to be in inst.

man

ok

vignette

BiocStyle doesn't render. Some images don't render either, please check the vignette to make sure it is as intended.

But very nice vignette otherwise, good job!

R

The utility functions you declare and use in the code, make it hard to follow your code. Mostly because they are redefining existing functions without improving efficiency and just adding an extra layer of complexity. Since it is not user facing, it's alright, but i'd highly prefer that you'd just use paste instead %++%. You especially lose the value of your %++% call when you are using that and paste in the same statement.

Also keep the same thing in mind for other functions within %...% syntax too.
Please follow coding style guidelines from http://bioconductor.org/developers/how-to/coding-style/. Using proper indentation really helps reviews look at your code. i'd like you to follow the style guide, as closely as possible before the next review.
In annotateVariationLength.R, your code is designed like,
```
if ...{ 
    return(out)
} 
if ...{
    return(out)
} else {
    ...
}
```
But, it's much clearer to design your statements like this,
```
if {
    ...
} 
if {
    ...
}
# No else case required
return(out)
```
The way the code is structured right now, makes it very difficult to follow the logic.

In this function, you also have many "magic" numbers, which are not documented. "3", "2" etc...it's hard to follow the logic. Please assign these numbers to a descriptive variable name and then use them. (Your future self will thank you as well.)

There are if, else patterns like this all over your code base, please fix all of them.
Accessors (simple functions that return components of your object) rather than direct slot access (using @) help to isolate the implementation of your class from its interface. Generally @ should only be used in an accessor, all other code should use the accessor. The accessor does not need to be exported from the class if the user has no need or business accessing parts of your data object.

Do this through out your code base, eg: cancerPanelClass.R
Remove commented out code.
Instead of using apply functions, please check out matrixStats package. You can use functions like rowAlls and colAlls which are vectorized and very efficient compared to apply statements.
```
rowidx <- apply(mat, 1 , function(x) all(x==0))
colidx <- apply(mat, 2 , function(x) all(x==0))
```

Check everywhere you use row-wise and col-wise operations, matrixStats is an amazing package which makes all these ops faster.

In this code chunk, what is the value of creating a new environment? Please explain,

    myenv <- new.env()
    dataExtractor(object=object , alterationType=alterationType 
            , tumor_type=tumor_type 
            , collapseMutationByGene=collapseMutationByGene 
            , collapseByGene=collapseByGene 
            , myenv=myenv , tumor.weights=tumor.weights)
  mydata <- get("mydata" , envir=myenv)
  mysamples <- get("mysamples" , envir=myenv)
  tum_type_diff <- get("tum_type_diff" , envir=myenv)
  rm(myenv)

Use BiocParallel and bplapply instead of mclapply. The vignette for BiocParallel is extensive and highly useful. Change the methods to use BiocParallel::bplapply. https://bioconductor.org/packages/release/bioc/html/BiocParallel.html

Not sure why you used both BiocParallel, and parallel interchangebly in the package.

README

The links in your readme are dead. Please replace them with active links, or remove them.

The shiny app you point to from your README file is also dead.

Please note that this is a first review. I'll review the code again, once you are ready with your changes.

gmelloni commented 5 years ago

Hi @nturaga , I am working on your comments, thanks a lot for taking the time to review my package.

I have a couple of questions:

Regarding the accessors, I would like to keep them exported to the user (I frequently play around wih part of the object myself). When I substitute the @ in the code for all the other functions I realize that sometimes I use the @ to modify the object, not to access it. If I create a replace method (e.g. cpData<-) I don't always modify the entire slot but only part of it and that raises an error when I validate the object:
```
# empty object looks like this
Formal class 'CancerPanel' [package "PrecisionTrialDrawer"] with 3 slots
@ arguments:List of 7
@ dataFull:List of 4
@ dataSubset:List of 5
```
This assignment works:
```
object@dataFull$mutations <- list(data=repos$mutations$data)
```
Using a replace method (cpData<-) raises an error:
```
cpData(object)$mutations <- list(data=repos$mutations$data)
invalid class "CancerPanel" object: CancerPanel slots are lists
```
Having a replace method would be desirable to customize little pieces of the object. Is there a way to get around this problem?
My second questions is if you can tell me which of the plots are not visible in the vignette. It seems to work for me.
This link is not dead to me: https://gmelloni.github.io/ptd/. Neither is the shiny app https://gmelloni.github.io/ptd/shinyapp.html (we just have some http/https problem with my old institution that I am trying to solve in these days).

gmelloni commented 5 years ago

In other words, related to the first question, is it ok to maintain the

object@slot$subslot <- value

in the code if it's not just an accessor but a replace method?

gmelloni commented 5 years ago

On top of that, using the accessor inside the code has a substantial overhead in terms of time:

microbenchmark( cpData(cpObj) , cpObj@dataFull )
Unit: nanoseconds
           expr   min      lq     mean median      uq    max neval cld
  cpData(cpObj) 21032 27340.5 33816.36  30362 35011.5 156652   100   b
 cpObj@dataFull    83   104.0   163.35    156   192.0    898   100  a

nturaga commented 5 years ago

@mtmorgan Might have more insight into the microbenchmark test you just responded with, regarding the slot access comparing @ with an accessor function.

The dead link might have been something on my end on that particular day, I now see that the shiny app doesn't load because of the problem you mentioned.

The webpage at https://shiny.bioinfo.ieo.it/app/ptd_studio might be temporarily down or it may have moved permanently to a new web address.

mtmorgan commented 5 years ago

Note the units -- nanoseconds. If that's really a concern, skip the explicit use of return() and use a plain-old-function (since there is only one method) rather than a generic.

your current package doesn't include an implementation of cpData<- and the example above isn't reproducible (don't know how to get 'repos'), but I think you want

setGeneric("cpData<-", function(object, value) standardGeneric("cpData<-"))
setReplaceMethod("cpData", "CancerPanel", function(object, value) {
    object@dataFull <- value
    object
})

which gives

> cpData(cpObj2)$simulations = list(data = letters)
> cpData(cpObj2)$simulations
$data
 [1] "a" "b" "c" "d" "e" "f" "g" "h" "i" "j" "k" "l" "m" "n" "o" "p" "q" "r" "s"
[20] "t" "u" "v" "w" "x" "y" "z"

> str(cpData(cpObj2))
List of 5
 $ fusions    :List of 2
  ..$ data   : NULL
  ..$ Samples: NULL
 $ mutations  :List of 2
  ..$ data   :'data.frame': 59 obs. of  9 variables:
  .. ..$ entrez_gene_id    : int [1:59] 7157 7157 7157 7157 7157 7157 7157 7157 7157 7157 ...
  .. ..$ gene_symbol       : chr [1:59] "TP53" "TP53" "TP53" "TP53" ...
  .. ..$ case_id           : chr [1:59] "TCGA-AB-2820" "TCGA-AB-2860" "TCGA-AB-2943" "TCGA-AB-2935" ...
  .. ..$ mutation_type     : chr [1:59] "Frame_Shift_Ins" "Frame_Shift_Ins" "Missense_Mutation" "Missense_Mutation" ...
  .. ..$ amino_acid_change : chr [1:59] "P223Rfs*4" "M40Lfs*7" "R273C" "R248Q" ...
  .. ..$ genetic_profile_id: chr [1:59] "laml_tcga_pub" "laml_tcga_pub" "laml_tcga_pub" "laml_tcga_pub" ...
  .. ..$ tumor_type        : chr [1:59] "laml" "laml" "laml" "laml" ...
  .. ..$ amino_position    : num [1:59] 223 40 273 248 193 176 317 126 179 280 ...
  .. ..$ genomic_position  : chr [1:59] "17:7578181:NA,GCGGCTC" "17:7579569:NA,CCATCCAG" "17:7577121:G,A" "17:7577538:C,T" ...
  ..$ Samples:List of 1
  .. ..$ laml: chr [1:200] "TCGA-AB-2803" "TCGA-AB-2804" "TCGA-AB-2806" "TCGA-AB-2815" ...
 $ copynumber :List of 2
  ..$ data   : NULL
  ..$ Samples: NULL
 $ expression :List of 2
  ..$ data   : NULL
  ..$ Samples: NULL
 $ simulations:List of 1
  ..$ data: chr [1:26] "a" "b" "c" "d" ...

bioc-issue-bot commented 5 years ago

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

4ae9ac4 add matrixStats in Depends 9fc941d add matrixStats functions 1cf72bc most lines reduced to 80 characters and code clean 6d7f954 bump 0.99.12

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.

gmelloni commented 5 years ago

Thanks @mtmorgan for taking the time to check this. Since the speed improvement is negligible I used the generic throughout the code. @nturaga I hope I addressed most of your comments, thanks again.

gmelloni commented 5 years ago

Hi @nturaga , any news? anything I can add?

nturaga commented 5 years ago

Hi @gmelloni , i'll take a look at your package by the end of the week. We have been busy with the new bioconductor release.

nturaga commented 5 years ago

Update

Plots in vignette don't render, example "3.1 coveragePlot". (attaching screenshot)

Same with 3.2, etc.

Actually except for 3.2 plot(myHTMLPlot), nothing else renders. Make sure they all render, because if the vignette is built and distributed as is, then all your users get a vignette with no figures. They would have to run the code again to figure out why they don't have rendered images. It could be because of the figure width/height options you have set, which makes them not render.
Otherwise your package looks in good shape to be accepted. Thanks for making all the changes.

Bioconductor / Contributions

PrecisionTrialDrawer submission #891

PrecisionTrialDrawer review

DESCRIPTION

NAMESPACE

data

inst

man

vignette

R

README

Update