Bioconductor / Contributions

Contribute Packages to Bioconductor
135 stars 33 forks source link

bayNorm #878

Closed WT215 closed 6 years ago

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

Hi @WT215

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: bayNorm
Type: Package
Title: Single-cell RNA sequencing data normalization
Version: 0.99.0
Authors@R: c(person("Wenhao", "Tang", role=c("aut", "cre"),
        email="wt215@ic.ac.uk"), 
        person("Fran<U+00E7>ois", "Bertaux", role=c("aut")),
        person("Philipp", "Thomas", role=c("aut")),
        person("Claire", "Stefanelli", role=c("aut")),
        person("Malika", "Saint", role=c("aut")),
        person("Samuel", "Marguerat", role=c("aut")),
        person("Vahid", "Shahrezaei", role=c("aut")))
Maintainer: Wenhao Tang <wt215@ic.ac.uk>
Description: bayNorm is used for normalizing single-cell RNA-seq data.
License: GPL (>= 2)
Encoding: UTF-8
LazyData: true
RoxygenNote: 6.1.0
Depends:
    R (>= 3.5),
Imports:
Rcpp (>= 0.12.12), 
BB,
foreach, 
iterators,
doSNOW,
parallel,
MASS,
locfit,
fitdistrplus,
stats,
            methods,
            graphics,
            grDevices,
            SummarizedExperiment,
            utils
LinkingTo: Rcpp, RcppArmadillo,RcppProgress
Suggests: 
knitr,
rmarkdown,
            BiocStyle,
            devtools,
            testthat
VignetteBuilder: knitr
biocViews: Normalization, RNASeq, SingleCell,Sequencing
URL: https://github.com/WT215/bayNorm
BugReports: https://github.com/WT215/bayNorm/issues

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

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: "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:

5cab86e Fix two warnings reported by Bioconductor

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:

f796fae 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:

39eace3 solve another warning in NEWS.Rd afc2f1e solve warnings

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.

lshep commented 6 years ago

Thank you for your submission to Bioconductor. Please see the following comments:

General: You need to provide a runnable executable example of your functions. It is unacceptable that all of your code is either in eval=FALSE in vignette or dontrun in man pages. Provide a small dummy set of data that will run in the time constraints. I recommend providing an example of using the SummarizedExperiment option for your functions as this is the common Bioconductor infastructure your package supports.

Build Report

README

DESCRIPTION

VIGNETTE

MAN

R Code

general

This could be simplified by assigning a variable the name of the function and then running with arguments which makes the code more readable

if(!mode_version & !mean_version){
    myFunc <- Main_NB_Bay
}else if(mode_version & !mean_version){
    myFunc <- Main_mode_NB_Bay
}else if(!mode_version & mean_version){
   myFunc <-  Main_mean_NB_Bay  
}

bayNorm_C_array<-myFunc(Data=synthetic_out$N_c,
            BETA_vec=synthetic_out$beta_c,size=size_c,
            mu=mu_c,S=1000,
            thres=max(synthetic_out$N_c)*2)

Also what about the last case where mode_version & mean_version. This does nothing but doesn't seem correct. If the user is not suppose to specify both, this should be checked in the code and mentioned in the documentation.

This should also be applied to other sections of code like the similar code in bayNorm.R - repeated code and similar code chunks should be avoided and simplified.

        if (!mode_version & !mean_version) {
            Bay_array <- Main_NB_Bay(
                Data = Data_sr,
                BETA_vec = BETA_vec,
                size = SIZE_input,
                mu = MU_input, S = S,
                thres = max(Data_sr)*2)
            rownames(Bay_array) <- rownames(Data)
            colnames(Bay_array) <- colnames(Data)

            return(list(
                Bay_array = Bay_array,
                PRIORS = PRIORS,
                input_params=input_params))
        } else if(mode_version){
            # mode
            Bay_mat <- Main_mode_NB_Bay(
                Data = Data_sr,
                BETA_vec = BETA_vec,
                size = SIZE_input,
                mu = MU_input, S = S,
                thres = max(Data_sr) *2)
            rownames(Bay_mat) <- rownames(Data)
            colnames(Bay_mat) <- colnames(Data)

            return(list(
                Bay_mat = Bay_mat,
                PRIORS = PRIORS,
                input_params=input_params))

        } else if(mean_version){
            # mean
            Bay_mat <- Main_mean_NB_Bay(
                Data = Data_sr,
                BETA_vec = BETA_vec,
                size = SIZE_input,
                mu = MU_input, S = 1000,
                thres = max(Data_sr) *2)
            rownames(Bay_mat) <- rownames(Data)
            colnames(Bay_mat) <- colnames(Data)

            return(list(
                Bay_mat = Bay_mat,
                PRIORS = PRIORS,
                input_params=input_params))

        }

OR another example where things can be simplified is this section in bayNorm where the DataList and BETAList are the same call - pull these output the if statement and only run the call that is needed in the conditional.

        if (is.null(UMI_sffl)) {
            # UMI
            DataList <- lapply(seq_along(Levels), function(x) {
                Data[, which(Conditions == Levels[x])]
            })
            DataList_sr <- lapply(seq_along(Levels), function(x) {
                Data[, which(Conditions == Levels[x])]
            })

            BETAList <- lapply(seq_along(Levels), function(x) {
                BETA_vec[which(Conditions == Levels[x])]
            })
        } else {
            # non-UMI

            DataList <- lapply(seq_along(Levels), function(x) {
                Data[, which(Conditions == Levels[x])]
            })
            DataList_sr <- lapply(seq_along(Levels), function(x) {
                round(Data[, which(Conditions == Levels[x])]/UMI_sffl[x])
            })
            BETAList <- lapply(seq_along(Levels), function(x) {
                BETA_vec[which(Conditions == Levels[x])]
            })
        }

Please apply the above corrections. Comment back here when the package is ready for a re-review with a summary of what has been changed.

Thank you and I look forward to working with you to get your package accepted to Bioconductor.

bioc-issue-bot commented 6 years ago

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

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

Congratulations! The package built without errors or warnings on all platforms.

Please see the build report for more details.

bioc-issue-bot commented 6 years ago

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

c5eb524 Further simplify code in PRIOR_FUNCTIONS.r

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.

WT215 commented 6 years ago

@lshep Hello, thank you for your review, it is very helpful! I have done the following changes:

  1. I use seq_len() instead of 1:....

  2. I added bioconductor installation instructions in both README.md and vignettes:

    source("https://bioconductor.org/biocLite.R")
    biocLite("bayNorm")

    Is this correct?

  3. Removed Maintainer in DESCRIPTION.

  4. Removed LazyData: true.

  5. The formatting of Imports, Suggests fields has been fixed.

  6. I removed not run and set eval=FALSE for the examples in bayNorm function and vignette respectively.

  7. In man page, for the object Data, I specified that the assays slot contains the expression matrix and is named "Counts".

  8. The code has been simplified in both noisygene_detection.r and bayNorm.r. Furthermore, I further simplified the code in PRIOR_FUNCTIONS.r.

  9. For the examples in vignette, I created a SummarizedExperiment object and then input it in the main functions.

  10. I added code for checking the basic setting at the very beginning such that when mode_version & mean_version is TRUE, bayNorm will stop and report error.

Thank you very much!

Cheers, Wenhao

lshep commented 6 years ago

The installation should use BiocManager and BiocInstaller is being deprecated

library(BiocManager)
BiocManager::install("bayNorm")

Thank you for the other corrections. I will re-review and comment back soon.

bioc-issue-bot commented 6 years ago

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

73e526b Using BiocManager

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

bioc-issue-bot commented 6 years ago

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

4d2f343 Rebuild locally then submit again

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.

WT215 commented 6 years ago

Hi @lshep, may I ask how is the re-review going? Is it possible for bayNorm to be included in the next release? Thank you very much!

bioc-issue-bot commented 6 years ago

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

250c10a Fix some errors

lshep commented 6 years ago

I apologize for the delay I will have a review for you tomorrow morning EST.

WT215 commented 6 years ago

Hi @lshep No problem! In the meantime I am also fixing some small errors in the code. Thank you for your help!

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.

lshep commented 6 years ago

Please see comments below.

General

vignette:

Thank you. I look forward to your comments.

WT215 commented 6 years ago

Hi @lshep, thank you for your re-review.

Regard to your General opinions:

The algorithms and comparison between bayNorm and other normalization methods (like SCnorm) have been posted in bioRxiv: Tang et al. (2018). bioRxiv. . We cited the paper in README. The vignette could be too verbose if we discussed details of algorithms there.

I will updated the vignette as soon as possible.

Thank you very much!

Best wishes, Wenhao

lshep commented 6 years ago

I would see if it is already accounted for by your support for SummarizedExperiment - since SingleCellExperiment extended SummarizedExperiment there may be nothing to be done on your part but to update the documentation saying that this class is also supported - but if your package is designed specifically for SingleCell Data than yes making sure it works with SingleCellExperiment would be the recommendation (please test this)

If there is no way to briefly summarized the algorithms used - I would at minimum also reference the paper in the vignette and package man page

When ready with updates don't forget to version bump for the new build report and comment back here

Cheers!

WT215 commented 6 years ago

Hi @lshep, no problem! I will reference the paper in both vignette and package man page. I just tested the SingleCellExperiment, it works fine. I will also included that class as you suggested.

I will let you know once it has been ready.

Thank you for your help!

Best wishes, Wenhao

bioc-issue-bot commented 6 years ago

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

612cf01 Allow more examples to be runnable; Update vignett...

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.

bioc-issue-bot commented 6 years ago

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

ae00b11 Fix error in vignette

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.

WT215 commented 6 years ago

@lshep Hi, may I ask how to fix this warning please? Warning: replacing previous import 'SingleCellExperiment::weights' by 'stats::weights' when loading 'bayNorm

I do not have SingleCellExperiment::weights in my code.

Thanks a lot!

lshep commented 6 years ago

You can ignore that warning for now - it has to do with the naming conflict of the function -

WT215 commented 6 years ago

Hi @lshep Ok I see. Then do I just leave it for you to have another review?

I have done the following changes:

  1. Including support of SingleCellExperiment class
  2. In vignette and man pages of bayNorm.R, I added the reference to the paper: Tang et al. (2018). bioRxiv.
  3. All examples are runnable except for some functions written in Rcpp. Because most of them need to be exported so that they can be included in the wrapper functions. However, for some main Rcpp functions, I also let the examples to be runnable.

Thanks a lot!

Best wishes, Wenhao

bioc-issue-bot commented 6 years ago

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

fe0f628 try to fix warning related to SingleCellExperiment

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:

264640d Fix Conditions option

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:

4eeef78 Correct error in noisy gene detection

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:

40f8973 Add Methods in vignettes

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.

WT215 commented 6 years ago

Hi @lshep , I have added Methods of bayNorm at the end of the vignettes. The only warning it reports is:

* checking whether package bayNorm can be installed ... WARNING
Found the following significant warnings:
  Warning: replacing previous import SingleCellExperiment::weights by stats::weights when loading bayNorm
See /Users/pkgbuild/packagebuilder/workers/jobs/878/40f8973/bayNorm.Rcheck/00install.out for details.

Please have a check.

Thanks a lot!

lshep commented 6 years ago

Looking good a few more comments:

R Files

PRIOR_FUNCTIONS.R


**Apply this concept throughout all R code**  
Like there seems to be similar structure in BB_Fun too - when possible mimize
code redundency

- [ ] bayNorm and bayNorm_sup have a lot of repeated code. It would be better to
  have all similar code in one function with a wrapper for the two different
  scenerios. Again standing behind limiting the amound of repeated code in
  packages. 

When ready with updates don't forget to version bump for the new build report and comment back here

Cheers!
bioc-issue-bot commented 6 years ago

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

124f01c Try to fix warning by using @importFrom SingleCell...

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:

02002fa Simplify code, update vignette