iCNV

[zhouzilu]

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

• Repository: https://github.com/zhouzilu/iCNV

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]

Hi @zhouzilu

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: iCNV
Title: Integrated Copy Number Variation detection
Version: 0.0.4
Author: Zilu Zhou, Nancy Zhang
Maintainer: Zilu Zhou <zhouzilu@pennmedicine.upenn.edu>
Description: Integrative copy number variation (CNV) detection from multiple platform and experimental design.
Depends: R (>= 3.3.1),
  CODEX
License: GPL-2
Encoding: UTF-8
LazyData: true
RoxygenNote: 6.0.1
Imports:
  fields,
  ggplot2,
  truncnorm,
  tidyr,
  data.table,
  dplyr 
Suggests: knitr,
    rmarkdown,
    WES.1KG.WUGSC
VignetteBuilder: knitr
biocViews: ExomeSeq, WholeGenome, SNP, CopyNumberVariation, HiddenMarkovModel

[bioc-issue-bot]

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]

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]

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

91ffabd version update

[bioc-issue-bot]

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]

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

237732f update version number

[bioc-issue-bot]

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]

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

6dab7ab Update

[bioc-issue-bot]

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]

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

3b1e394 Update-warningfix

[bioc-issue-bot]

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]

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

c9e48a4 fixwarnings

[bioc-issue-bot]

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.

[zhouzilu]

@Liubuntu The only warning I receieved is the R version update. In order for this package to be applicable to most of the user, I decided to maintain the requred R version to 3.3.1. Is that possible?

[bioc-issue-bot]

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

b28de4a version pump

[bioc-issue-bot]

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.

[zhouzilu]

@Liubuntu Just to make sure it doesn't swamp by the bot. The only warning I receieved is the R version update. In order for this package to be applicable to most of the user, I decided to maintain the requred R version to 3.3.1. Is that possible?

[Liubuntu]

Hi @zhouzilu , Please do the following Update R version dependency from 3.3.1 to 3.5. in DESCRIPTION file. Because the newly accepted packages will go to the Bioc 3.7 devel, which is supposed to work interactively with the R version of 3.5 and up. The Bioc versions are bumped in accordance with the R versions. Thanks!

[mtmorgan]

Just to clarify -- specifying R 3.5 is highly recommended, but not strictly required.

[zhouzilu]

@mtmorgan @Liubuntu Thank you for clarifying this, Martin and Qian. After talked with other author of this package, we have decide to keep our R version as R 3.3.1 due to

• Most of the available computing cluster is not R 3.5
• This package is built based on R 3.3.1 environment
• Our downstream analysis will be done in a computer cluster that has R 3.3.1

Hope this address your concern. In addition, the related paper for this package is going to be finilized in three days. I wonder if we could accelarate the package review process, such that I could include the Bioconductor link in this paper.

Please let me know if you need any other addtional information to proceed.

[Liubuntu]

Hi @zhouzilu ,

Here is the initial review of your package. Please seek to fix all of them and bump the version for a new build. A normal technical review will take about 2 weeks to a month (or longer) depending on the responsiveness and package fixes. The reviewers in the core team has other important responsibilities from Bioconductor. But we could try to accelerate the process working together more efficiently.

DESCRIPTION

• ok.

  NAMESPACE

• Please import all the packages used in your Depends and Imports in DESCRIPTION file. See those no visible global function definition for ... in your build report?Most of those could go away once you use @import or @importFrom inside your R scripts.

  Undefined global functions or variables:
      #CHROM ALT FILTER FORMAT INFO QUAL REF aggregate axis bafs
      colorRampPalette dev.off dnorm dunif grid kmeans legend par pdf pib
      pir plot points read.table sd write.table zs
  Consider adding
  importFrom("grDevices", "colorRampPalette", "dev.off", "pdf")
  importFrom("graphics", "axis", "grid", "legend", "par", "plot",
                   "points")
  importFrom("stats", "aggregate", "dnorm", "dunif", "kmeans", "sd"
  importFrom("utils", "read.table", "write.table")
  to your NAMESPACE file.

Vignette

• The author, title and dates are duplicated.
• Please add a table of contents for better organization. This could be easily specified in the header. check here: https://rmarkdown.rstudio.com/html_document_format.html

---
title: ...
output:
  html_document:
      toc: true
---

• you can keep the demo for installing the most current version from github. But please also include demo for how to load the package from Bioconductor in the vignette, as if it is already available from Bioconductor.
• Where is the red box? This package specifically emphasizes on the steps within the red box..
• some figures are too big. Check out here: https://sebastiansauer.github.io/figure_sizing_knitr/ to set figure sizes within rmarkdown.
• Add sessionInfo() in the end of vignette for the info of your R environment.
• Multiple typos. E.g., typo in "We will seperately shows normalization for WES and SNP array." and "At this step, we should alreday have PLR and variants BAF from sequencing, ...". "This is mainly to accomadate the fact that ...". You may want to run a spell checker.

R

Please fix the following first, and a closer check of R scripts might be given later.

• When there are default values for function arguments, please specify them in the @parameter documentation.

The following are for a more robust R coding practice:

• use <- for assignment. E.g., iCNV_detection = function to iCNV_detection <- function(). Check code inside the functions also.
• Avoid using of sapply, use vapply instead to be safer. You can also replace it with lapply if originally sapply(simplify=FALSE).
• Avoid using of 1:..., use seq_len or seq_along instead so that your code won't break for edge cases. E.g., in "iCNV_detection_function.R:L529", for (j in 1:length(CNVs)){ could be for (j in seq_len(CNVs)){. Search your R scripts for similar cases.
• Long lines. E.g., in "iCNV_detection_function.R". Please use new lines and tab for indentation. Use multiples of 4 for indentation.

others

• This is not required. But consider adding unit tests. We strongly encourage them. See http://bioconductor.org/developers/how-to/unitTesting-guidelines/.

Cheers!

[zhouzilu]

@Liubuntu Thank you for your comment. I will edit my package accordingly and bump the version once finished.

[bioc-issue-bot]

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

1576bfd version bump * version bump to 0.99.14

[bioc-issue-bot]

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]

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

f37a3c0 Debug - Debug vapply - remove pir, pib - add i...

[bioc-issue-bot]

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]

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

84fc88e Fix bug Fix bug in the vignette header

[bioc-issue-bot]

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]

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

ba9d897 fix bug fix bug and bump version

[bioc-issue-bot]

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]

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

2c77c82 Note Fix Indentation fix to space sapply fi...

[bioc-issue-bot]

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]

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

50f2069 Bug fix - add ; - shorter lines

[bioc-issue-bot]

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]

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

3dbd42b Version bump, Rd update and bug fix

[bioc-issue-bot]

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.

[zhouzilu]

@Liubuntu Hi Qian, I have finished fixing most of the issue you mentioned except the 4 space indent. This seems a little bit trivial... Is it fine just keep it that way? Other than that, it's all finished. Thank you again!

[Liubuntu]

Hi @zhouzilu ,

Thanks for making those changes to the review. It will surely make your package a better one with more users. I will take another look at your newly modified package and give another review very soon.

First of all, the indentation thing is not strictly required, but we highly recommend for the good R coding styles.

Actually, you can use "tab" key in emacs and it will automatically give you correct indentation. Also for longer lines (> 80 characters), you can use "alt+q" to do the automatic reformatting of the line lengths. OR if you are using Rstudio, the indentation could be done automatically when you goes to the next line ("tab" could also be used).

You can refer to the following sites for coding styles: https://www.bioconductor.org/developers/how-to/coding-style/ http://r-pkgs.had.co.nz/style.html

Like Hadley said in his article, "Good style is important because while your code only has one author, it’ll usually have multiple readers." You really should use a consistent style to make it a better and more readable package for a broader users.

Thanks! Qian

[zhouzilu]

Hi @Liubuntu ,these are really great comments! I will be careful with all the details when I developed my second package. Thank you again!

[Liubuntu]

Hi @zhouzilu ,

Thanks for making your efforts in modifying your package. Here are some additional things to do, but most are just cosmetic. Please bump your version and comment back when you are ready for the final review or with any questions. Thanks!

NAMESPACE

• you need to import CODEX in namespace! Remember to import all packages that are mentioned in Depends and Imports fields of DESCRIPTION. Or remove any unused ones from DESCRIPTION.

no visible binding problem.

There are still this note in build report for no visible binding for global variable .... These comes from the using of dplyr. Please use .data$ before each use of these variable. E.g., rewrite:

bambaf_from_vcf.R:    dt <- dt %>% dplyr::filter(QUAL!='.') %>% dplyr::select(-REF,-ALT,-QUAL,-FILTER,-INFO)

into:

bambaf_from_vcf.R:    dt <- dt %>% dplyr::filter(.data$QUAL!='.') %>% dplyr::select(-.data$REF,-.data$ALT,-.data$QUAL,-.data$FILTER,-.data$INFO)

And import .data from rlang. Remember to add rlang into your DESCRIPTION in Imports field. This will remove the build check messages:

More information, please check https://community.rstudio.com/t/tidyeval-equivalent-of-mutate/1295/5

build report notes:

bam.baf: no visible binding for global variable QUAL
bam.baf: no visible binding for global variable REF
bam.baf: no visible binding for global variable ALT
bam.baf: no visible binding for global variable FILTER
bam.baf: no visible binding for global variable INFO
bam.baf: no visible binding for global variable FORMAT
bambaf_from_vcf : <anonymous>: no visible binding for global variable
  #CHROM

Search through your source code for all these cases. These are the recommended way for doing it correctly. Try your best, and comment back for any question.

R scripts

• Since the argument of dir, pattern and projectname are character strings as said in your parameter documentation, it is recommended to write: get_array_intput <- function(dir,pattern,chr=NULL,projectname='') as:

get_array_intput <- function(dir=character(),pattern=character(),chr=NULL,projectname=character())

and it will do the automatic check for your input and return error if the input is not in character. You can also use integer() or numeric() for integer or numeric arguments. Search and correct for other similar cases.

Vignette

• looks much better now! Thanks!

Cheers! Qian

[zhouzilu]

@Liubuntu Thanks, Qian! I will modify accordingly, bump the version and contact you once finished.

[zhouzilu]

@Liubuntu Quick question about auto check for input type in function. Seems like it doesn't work that way. Do you mind send me your reference? I may have made a mistake somewhere. Also, if I have a default value, for example '.', should I use dir = character('.') Thank you!

[Liubuntu]

Hi @zhouzilu ,

That was from my misunderstanding and sorry for passing that information without checking the validity.

But indeed, it's a better practice to give a default value (of the required data type) for function arguments, and also add some checking for input in the beginning of the function. E.g.,

foo <- function(x=character(), y=numeric())
{
    stopifnot(is.character(x))
    stopifnot(is.numeric(y))
    message("name: ", x, "\n", "Age: ", y, "\n")
}

So that the input value of wrong data type should return an error.

foo("Mei", 23)
## name: Mei
## Age: 23
foo(22, 52)
## Error in foo(22, 52) : is.character(x) is not TRUE
foo("Mei", "a")
## Error in foo("Mei", "a") : is.numeric(y) is not TRUE

And also when users types in args(foo), it will return the arguments with default values, which could serve as a reminder for the correct data type. While it's always useful to specify the required data type in the @param documentation also.

## function (x = character(), y = numeric())
## NULL

This will keep you code robust. You can also add unit tests for these testings and more. We strongly encourage them. See http://bioconductor.org/developers/how-to/unitTesting-guidelines/.

For your other question, default values like "." does not need to specify the data type within the argument. But please include it in the @param documentation.

Best, Qian

[Liubuntu]

Hi @zhouzilu ,

About the "no visible binding problem" in my previous review, there has been discussions within the Bioconductor core team. There are multiple ways of dealing with it, including adding the pronoun as I commented from Hadley's solution, or using "utils::globalVariables" to mute those note messages (which is not recommended, and could be masking of any inappropriate use of that variable), or just prevent using the pipe syntax (Because the pipe syntax is convenient way for end-users, but not appropriate for lower-level programming). There was not a conclusion yet, but soon some guideline about using the pipe syntax in writing an R package will be added.

To simplify, since you are already using this pipe syntax in your own package, and it looks tedious to add the pronoun before each call of the data fields. Although the guideline of reviewing Bioconductor packages is to take care of things that are under control, I would say it is OK to leave those note messages unattended. It should be much simpler to address the rest of issues that are more cosmetic.

Cheers! Qian

[Liubuntu]

Hi @zhouzilu ,

Please make effort in addressing the issues in the last review. Read the comments and adjust accordingly. Since most of the needed things are just cosmetic and could be easy to fix. Bioconductor usually has a 2 weeks rule, and if no reponse (comments / questions / new commits or anything showing your are working to improve your package), the issue will be closed. It was 8 days ago for your last response. So please response within a week so that we can keep this issue open and the package is already very close to acceptance. Thanks!

Best, Qian

[zhouzilu]

Hi @Liubuntu Sorry. I was working on my other project. I will work on it during the weekend and push the update.

Thanks, Zilu

[zhouzilu]

@Liubuntu Qian, really quick question. So I understand I need to add CODEX to the NAMESPACE. However, since CODEX is a Bioconductor package, how should I import it? Is it different from other R packages?

Thank you!

[bioc-issue-bot]

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

dc84674 Update according to Bioconductor - import rlang -...

[bioc-issue-bot]

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]

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

32f877a add rlang to description