Bioconductor / Contributions

Contribute Packages to Bioconductor
134 stars 33 forks source link

RadioGx resubmission #1490

Closed ChristopherEeles closed 4 years ago

ChristopherEeles commented 4 years ago

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]'

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.

bioc-issue-bot commented 4 years ago

Hi @ChristopherEeles

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: RadioGx
Type: Package
Title: Analysis of Large-Scale Radio-Genomic Data 
Version: 0.99.3
Date: 2020-03-30
Authors@R: c(
  person("Venkata","Manem", email = "mail2mvskumar@gmail.com", 
    role = c("aut")),
  person("Petr","Smirnov", email = "petr.smirnov@uhnresearch.ca", 
    role = c("aut")),
  person("Ian","Smith", email = "ianc.smith@mail.utoronto.ca", 
    role = c("aut")),
  person("Meghan","Lambie", email = "megan.lambie@mail.utoronto.ca", 
    role = c("aut")),
  person("Christopher","Eeles", email = "christopher.eeles@uhnresearch.ca",
    role = c("aut")),
  person("Scott", "Bratman", email = "scott.bratman@rmp.uhn.ca", 
    role = c("aut")),
  person("Benjamin","Haibe-Kains", email = "benjamin.haibe.kains@utoronto.ca",
    role = c("aut","cre"))
  )  
Description: Computational tool box for radio-genomic analysis which integrates
    radio-response data, radio-biological modelling and comprehensive cell line
    annotations for hundreds of cancer cell lines. The 'RadioSet' class enables
    creation and manipulation of standardized datasets including information
    about cancer cells lines, radio-response assays and dose-response indicators. 
    Included methods allow fitting and plotting dose-response data using 
    established radio-biological models along with quality control to validate
    results. Additional functions related to fitting and plotting dose response 
    curves, quantifying statistical correlation and calculating area under the
    curve (AUC) or survival fraction (SF) are included. For more details please 
    see the included documentation, references, as well as:
    Manem, V. et al (2018) <doi:10.1101/449793>.
License: GPL-3
VignetteBuilder: knitr
VignetteEngine: knitr::rmarkdown
Encoding: UTF-8
LazyData: true
Depends: R (>= 4.0), CoreGx
Imports:
    SummarizedExperiment,
    S4Vectors,
    Biobase,
    parallel,
    BiocParallel,
    RColorBrewer, 
    caTools, 
    magicaxis, 
    methods, 
    reshape2,
    scales,
    grDevices,
    graphics,
    stats,
    utils,
    assertthat,
    matrixStats
Suggests:
    rmarkdown,
    BiocStyle,
    knitr,
    pander
biocViews: Software, Pharmacogenetics, QualityControl, Survival
RoxygenNote: 7.1.0

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 4 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 4 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.

nturaga commented 4 years ago

RadioGX review

R CMD build

ok

R CMD INSTALL

ok

DESCRIPTION

ok

NAMESPACE

ok

vignette

R

        * NOTE: Avoid sapply(); use vapply()
            Found in files:
              computeSensitivity.R (line 94, column 12)
              computeSensitivity.R (line 95, column 11)
              doseResponseCurve.R (line 287, column 18)
              geneDrugSensitivity.R (line 36, column 27)
              plotCurve.R (line 96, column 13)
              radSensitivitySig.R (line 195, column 24)
              radSensitivitySig.R (line 214, column 17)
              radSensitivitySig.R (line 220, column 14)
              rankGeneDrugSensitivity.R (line 104, column 24)
              rankGeneDrugSensitivity.R (line 107, column 25)
              rankGeneDrugSensitivity.R (line 121, column 26)
  1. There are also no test cases for such an extensive codebase in this package. testthat makes it very easy to write test cases.

    Please consider writing some test cases.

  2. As I mentioned about the formatting, the build report throws the same as a NOTE,

        * Checking formatting of DESCRIPTION, NAMESPACE, man pages, R source,
        and vignette source...
          * NOTE: Consider shorter lines; 345 lines (6%) are > 80 characters
            long.
          First 6 lines:
            R/computeAUC.R:82     CoreGx::.sanitizeInput(pars = pars, # Added this ...
            R/computeAUC.R:107       return(pars[[1]] / 2 * (lower ^ 2 - upper ^ 2)...
            R/computeAUC.R:113           return((exp(-pars[[1]] * lower) - exp(-par...
            R/computeSensitivity.R:1 ## This function computes pars/AUC/SF2/D10 for...
            R/computeSensitivity.R:28     pars <- lapply(names(AUC), function(exp, ...
            R/computeSensitivity.R:29       if(length(grep("///", raw.sensitivity[e...
          * NOTE: Consider 4 spaces instead of tabs; 32 lines (1%) contain
            tabs.
          First 6 lines:
            R/RadioSetClass.R:1269      exps <- adArgs[["exps"]]
            R/RadioSetClass.R:1270      if(is(exps,"data.frame")){
            R/RadioSetClass.R:1271          exps2 <- exps[[name(object)]]
            R/RadioSetClass.R:1272          names(exps2) <- rownames(exps)
            R/RadioSetClass.R:1273          exps <- exps2
            R/RadioSetClass.R:1274      } else{
          * NOTE: Consider multiples of 4 spaces for line indents, 1399
            lines(23%) are not.
          First 6 lines:
            R/computeAUC.R:38                        SF_as_log = FALSE,
            R/computeAUC.R:39                        area.type = c("Fitted", "Actua...
            R/computeAUC.R:40                        verbose = TRUE)
            R/computeAUC.R:41   {
            R/computeAUC.R:42   area.type <- match.arg(area.type)
            R/computeAUC.R:44   if (!missing(SF)) {
          See http://bioconductor.org/developers/how-to/coding-style/
          See FormatR package:
            https://cran.r-project.org/web/packages/formatR/index.html

Based on your previous response, (during the CoreGx package review, and I asked to uniformly fix them across all the packages). It seems like some of these issues which I raised in my last review still persist. You spoke of an upcoming "refractor". Any idea when this is going to happen? Please keep in mind, when you have such a massive code refractor to improve the codebase , why not do it before the review and make it easier for the reviewer as well?

    Coding style: I realize that some of our functions are hard to
    read, we are in the process of refactoring all four packages
    (CoreGx, PharmacoGx, RadioGx, ToxicoGx) and plan to get the
    codebase in shape within a month or two. Until then please let me
    know if there are any style issues I should address immediately

General

Since you are aiming to get this in for the conference, please make as many updates as you can and leave a reply on each point detailing what is possible and what isn't at the moment. That way we can move forward to package acceptance in a faster way.

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

bioc-issue-bot commented 4 years ago

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

8abe224 Update: Responding to Bioconductor review ef782e3 Update: Responding to Bioconductor review f9d86cd Merge branch 'development'

bioc-issue-bot commented 4 years ago

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

a14eb57 Update: Version bump to trigger build

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

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

ChristopherEeles commented 4 years ago
  1. Vignette

    • Expanded current vignette to include computation of common radiation dose-response metrics from a RadioSet and a fitted linear-quadratic model.
    • Next steps for the vignette is to add section on detecting molecular signatures of radiation response, this will be written over the next month as we prepare for our workshop at Bioc 2020.
  2. Replace

    • Line now reads: D <- seq(0, 10 ,1) and comments removed
  3. Formatting inconsistency

    • I agree that the formatting consistency is an issue; the codebase was developed by a number of summer and graduate students over several years. I working on cleaning it up but am currently the only full-time R developer in our lab so it will take some time.
    • Noted the 4 space issue; I tried to fix with quick regex but it didn't work; this will be my first formatting priority for all Gx packages.
  4. Notes

    • I believe I have removed all sapply statements
    • Shorter lines and shorter functions are an item on the list for the refactor, but the first steps are going to be minimizing redundancy between all Gx packages
    • I expect we will end up throwing away a lot of code, so I feel I should address formatting after we finish rewriting longer functions
  5. Tests

    • I also agree tests are important, I think it makes the most sense to start with CoreGx since those functions are inherited by all Gx packages
    • Unless you think it is essential I would prefer to address the unit testing issue in the future (once we have finished minimizing redundancy between packages and rewriting function
  6. Refactor

    • RadioGx and ToxicoGx were already on CRAN, we had hoped to get them into Bioconductor quickly then start the refactor with CoreGx -> PharmacoGx -> RadioGx -> ToxicoGx
    • Still planning a major refactor, but I am the only full-time developer working on this and I have to balance my effort between several projects so it will take some time
    • Petr Smirnov has already started refactoring functions in PharmacoGx, which will be propagated back into CoreGx as we continue to clean up the code-base
  7. @ Access

    • I have removed all @ slot access outside of accessor methods
  8. RadioSet

    • I broke up the class from its accessor methods, it should be easier to deal with now
    • I will do the same in other Gx packages as well
bioc-issue-bot commented 4 years ago

Your package has been accepted. It will be added to the Bioconductor Git repository and nightly builds. Additional information will be posed to this issue in the next several days.

Thank you for contributing to Bioconductor!

mtmorgan commented 4 years ago

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/ChristopherEeles.keys is not empty), then no further steps are required. Otherwise, do the following:

  1. Add an SSH key to your github account
  2. Submit your SSH key to Bioconductor

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 BiocManager::install("RadioGx"). The package 'landing page' will be created at

https://bioconductor.org/packages/RadioGx

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.