search

Bioconductor / Contributions

Contribute Packages to Bioconductor
134 stars 33 forks source link

breakpointRdata #835

Closed daewoooo closed 6 years ago

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

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: breakpointRdata
Type: Package
Title: Strand-seq data for demonstration purposes
Version: 0.99.0
Date: 2016-08
Author: Aaron Taudt, David Porubsky
Maintainer: David Porubsky <david.porubsky@gmail.com>
Description: Strand-seq data to demonstrate functionalities
    of breakpointR package.
Depends:
  R (>= 3.4)  
Suggests:
  knitr,
  BiocStyle, 
License: file LICENSE
VignetteBuilder: knitr
biocViews: ExperimentData, Homo_sapiens_Data, SequencingData, DNASeqData
NeedsCompilation: no
URL: https://github.com/daewoooo/breakpointRdata
RoxygenNote: 6.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: "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:

bb8bd9b bug fix: fixed R version in DESCRIPTION

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.

daewoooo commented 6 years ago

AdditionalPackage: https://github.com/daewoooo/breakpointR

bioc-issue-bot commented 6 years ago

Hi @daewoooo,

Starting build on additional package https://github.com/daewoooo/breakpointR.

IMPORTANT: Please read the instructions for setting up a push hook on your repository, or further changes to your additional package repository will NOT trigger a new build.

The DESCRIPTION file of this additional package is:

Package: breakpointR
Type: Package
Title: Find breakpoints in Strand-seq data
Version: 0.99.0
Date: 2015-10-05
Author: David Porubsky, Ashley Sanders, Aaron Taudt
Maintainer: David Porubsky <david.porubsky@gmail.com>
Description: This package implements functions for finding breakpoints, plotting and export of Strand-seq data.
Depends:
  R (>= 3.4.0),
  GenomicRanges,
  cowplot,
  breakpointRdata
Imports:
  methods,
  utils,
  grDevices,
  stats,
  S4Vectors,
  GenomeInfoDb (>= 1.12.3),
  IRanges,
  Rsamtools,
  GenomicAlignments,
  ggplot2,
  BiocGenerics,
  gtools,
  doParallel,
  foreach
Suggests:
  knitr,
  BiocStyle,
  testthat
License: file LICENSE
LazyLoad: yes
VignetteBuilder: knitr
Packaged: 2015-09-19 13:29:00 CET+1; Taudt
biocViews: Software, Sequencing, DNASeq, SingleCell, Coverage
URL: https://github.com/daewoooo/BreakPointR
RoxygenNote: 6.0.1
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: "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:

4e24b8d resolved bugs and updated package version

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:

788c795 fixed bugs reported by bioc build system

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:

631556a fixed bugs reported by bioc build system

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:

ccc72bc fixed bugs reported by bioc build system

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:

1c35104 fixed problems reported by bioc build system

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:

7727a53 fixed problems reported by bioc build system

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:

ddb6b1d fixed problems reported by bioc build system

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:

24a1845 fixed problems reported by bioc build system

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:

a8a1753 Fixed problems reported by bioc build system

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

1f064f5 Fixed problems reported by bioc build system

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.

dvantwisk commented 6 years ago

Here is the review for this package. It's a data package, so there's not much to say but there are a few small things. The [REQUIRED] tags indicate changes that must be made or require an explanation. The [CONSIDER] tags indicate changes that are advisable but not required. Please message me back when you are done making changes.

DESCRIPTION:

R/documented_data.R

daewoooo commented 6 years ago

Hi Daniel,

Thank you very much for your comments to improve the quality of this package. Please find my response to your comments below. [REQUIRED] I have modified the LICENSE file to comply with the standard MIT LICENSE.

[CONSIDER] I have modified R/documented_data.R to provide more details on the data objects presented with this data package.

For example_bams I have added the reference genome the data were aligned to and also what modifications have been made to the data in order to reduce the file size.

For example_results I have a added a description of the overall structure of the RData object. Also I have added a description of each element of this RData object.

Best regards,

David

bioc-issue-bot commented 6 years ago

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

9094a12 Resolved issues pointed out by the reviewer

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.

dvantwisk commented 6 years ago

Hi, The changes you made to the package look fine. I am ready to accept if you are.

daewoooo commented 6 years ago

Hi Daniel,

Sounds good to me. Thanks again for your review. I assume that the review of the main package (breakpointR) will follow.

David

dvantwisk commented 6 years ago

I'll accept the package then. As for the breakpointR package. I'm not seeing it as an open issue. Do you have a link to that issue?

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

daewoooo commented 6 years ago

Nice thanks. breakpointR package was added as an additional package to this issue 14days ago. See the link. https://github.com/Bioconductor/Contributions/issues/835#issuecomment-412973692 I hope I did right. Let me know if you have any trouble to find it.

dvantwisk commented 6 years ago

I suppose I didn't see it! I will get a review of this to you soon.

lshep commented 6 years ago

Hi @daewoooo For the record: Yes you did do this correctly by submitting the data package first and the software package as an additional package. Please see the following review of the software package:

General:

README

etc... 


**NEWS**
- [ ] Thank you for including the NEWS file but it is formatted incorrectly. Please have headers in the form of "Changes in version x.y.z"  and the items below will have to be identified with either a -, +, or o ... so for instance the NEWS file would become

Changes in version 0.99.0


**LICENSE**
- [ ] I see you included your own license - it doesn't restrict distribution or use so its fine but I was just curious why a standard open source license like those already include with R couldn't be used? I see that you distributed the data package under MIT - was there a reason it couldn't be the same?

**test/testthat**
- [ ] You included RData files in your testthat so the tests are really not interactively testing the functions in your package.  The point of the tests is so the files are generated and run on freshly generated data to indicate any problems. The way its implemented is really tricking the testing process

**vignette* 
- [ ] I suggest removing the -knit from the vignette title.  Generally the vignettes are the name of the package or something meaniful as within R the user will call vignette("vignetteTitle")  it is not very intuitive to be vignette("breakpointR-knitr")  

- [ ] your main function breakpointr is never run as in the man/example file it is in a dontrun{} and in the vignette is in included in code chunks that are eval=FALSE.  We require that all functions are evaluated at least once for testing. 

- [ ] Actually most of the code chunks in your vignette are eval=FALSE; This is unacceptable as the vignette needs a working example.  If you are including a data package you can and should be including files to allow your package to have a working example (unless using already existing Experiment Data from other packages which is strongly encouraged).  There are BAM files available through existing Bicondcutor Experiment Data packages like [pasillaBamSubset](https://www.bioconductor.org/packages/release/data/experiment/html/pasillaBamSubset.html) 
There are also newly submitted BAM files on ExperimentHub, althoughth that package is also going throught the submission process they can be accessed with the devel experimenthub

eh = ExperimentHub() qusnapshotDate(): 2018-09-05 query(eh, "MMAPPR2data") ExperimentHub with 4 records

snapshotDate(): 2018-09-05

$dataprovider: Bench to Bassinet GnomEx CvDC

$species: Danio rerio

$rdataclass: character

additional mcols(): taxonomyid, genome, description,

coordinate_1_based, maintainer, rdatadateadded, preparerclass, tags,

rdatapath, sourceurl, sourcetype

retrieve records with, e.g., 'object[["EH1657"]]'

       title

EH1657 | zy13mut
EH1658 | zy13mutIdx EH1659 | zy13wt
EH1660 | zy13wtIdx 


Please update the vignette to have a working example of your functions.  

**R code**
- [ ] In all files - you nest internal helper functions inside other functions which make it very hard to read. I suggest making an internal.R or utils.R that contains all these internal helper functions to allow the function calls to only include the relevant code. 

- [ ] writeBed.R -  is this really needed?  Why could you not use the export function provided by rtracklayer? rtracklayer has an export method that will write BED files. 

- [ ] importReads.R - similar to above.  There are already existing methods for reading BAM files. 

breakpoint.R 
- [ ] Don't repeat code - example in the section commmented #binning#  The code is essentially repeated with few changes between whether createCompositFile, numCPU >1 and else - have a function that defines the main section of this code and these sections become wrappers around it. 

Please start with these suggested changes.  Please comment back here addressing the above issues and when you would like a re-review of the package. 

Thank you and Thank you for your efforts and interest in Bioconductor.
bioc-issue-bot commented 6 years ago

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

2f06dc0 Version bump

daewoooo commented 6 years ago

Dear reviewer,

Thank you very much for the review of breakpointR package. I appreciate your effort and time to make this package better. I have addressed your comments one-by-one below (text in italics). Updated version of the package have pushed to git.

General: Please clean up NOTES in the build report

Warning in readLines(infile) :

README Please use BiocManager::install for installing from bioconductor and not Biocinstaller::biocLite if (!requireNamespace("BiocManager", quietly=TRUE)) install.packages("BiocManager") install("GenomicRanges")

etc... We have modified the README file to meet the above mentioned requirements.

NEWS Thank you for including the NEWS file but it is formatted incorrectly. Please have headers in the form of "Changes in version x.y.z" and the items below will have to be identified with either a -, +, or o ... so for instance the NEWS file would become Changes in version 0.99.0

LICENSE I see you included your own license - it doesn't restrict distribution or use so its fine but I was just curious why a standard open source license like those already include with R couldn't be used? I see that you distributed the data package under MIT - was there a reason it couldn't be the same? We have changed the license in order to comply with MIT standards.

test/testthat You included RData files in your testthat so the tests are really not interactively testing the functions in your package. The point of the tests is so the files are generated and run on freshly generated data to indicate any problems. The way its implemented is really tricking the testing process In order to save space we have stored raw Strand-seq data (raw sequecning reads) as a GRanges object and saved it in RData file. Such raw reads are then processed using runBreakpoinr function to test all main functionalities of breakpointR package to detect breakpoints in Strand-seq data. Running this function we rigorously test if the whole breakpoint detection procedure will give us an expected number of breakpoints and as such uncover any bugs in our code.

*vignette I suggest removing the -knit from the vignette title. Generally the vignettes are the name of the package or something meaningful as within R the user will call vignette("vignetteTitle") it is not very intuitive to be vignette("breakpointR-knitr") As suggested, we have changed the name of vignettes to breakpointR.Rnw .

your main function breakpointr is never run as in the man/example file it is in a dontrun{} and in the vignette is in included in code chunks that are eval=FALSE. We require that all functions are evaluated at least once for testing. As mentioned before, the main breakpointr function is now fully run as a vignette example.

Actually most of the code chunks in your vignette are eval=FALSE; This is unacceptable as the vignette needs a working example. If you are including a data package you can and should be including files to allow your package to have a working example (unless using already existing Experiment Data from other packages which is strongly encouraged). There are BAM files available through existing Bioconductor Experiment Data packages like pasillaBamSubset We have added full runnable example of the main breakpointR function that runs whole breakpoint detection pipeline. Vignettes also contain explanations of recommended setting for the main breakpointr function. These explanations are eval=FALSE as we think it’s unnecessary to run the same function over and over again.

There are also newly submitted BAM files on ExperimentHub, althoughth that package is also going throught the submission process they can be accessed with the devel experimenthub Unfortunately, none of the above mentioned examples can be used to test functionalities of breakpointR package. breakpointR is specifically designed to process single-cell Strand-seq data.

eh = ExperimentHub() qusnapshotDate(): 2018-09-05 query(eh, "MMAPPR2data") ExperimentHub with 4 records

snapshotDate(): 2018-09-05

$dataprovider: Bench to Bassinet GnomEx CvDC

$species: Danio rerio

$rdataclass: character

additional mcols(): taxonomyid, genome, description,

coordinate_1_based, maintainer, rdatadateadded, preparerclass, tags,

rdatapath, sourceurl, sourcetype

retrieve records with, e.g., 'object[["EH1657"]]'

       title

EH1657 | zy13mut
EH1658 | zy13mutIdx EH1659 | zy13wt
EH1660 | zy13wtIdx

Please update the vignette to have a working example of your functions. We changed the vignettes to contain runnable examples of the main function to ensure that the whole breakpoint analysis can be run. Other parameter explanations for the main function are not runnable since they only point user how to use those parameters.

R code In all files - you nest internal helper functions inside other functions which make it very hard to read. I suggest making an internal.R or utils.R that contains all these internal helper functions to allow the function calls to only include the relevant code. We made sure there are no undefined internal function within other functions. Also we have created new utils.R file for any helper function used throughout the package.

writeBed.R - is this really needed? Why could you not use the export function provided by rtracklayer? rtracklayer has an export method that will write BED files. As suggested we have further explored the possibility to utilize rtracklayer functionalities. However, we concluded that for our purposes implementation of rtracklayer would not bring much improvement in readability of our bed export function. Therefore we opt for splitting our main export function in two function to make our code more concise and easy to understand.

importReads.R - similar to above. There are already existing methods for reading BAM files. Unfortunately, existing methods are not applicable for specifics of Strand-seq data. In our function we have to make sure that the read directionality in every single library is preserved regardless of single or paired-end data being loaded. Moreover, we offer user additional options, such as merging paired-end reads into a single fragment while preserving read directionality as well as optional filtering of alternative alignments.

breakpoint.R Don't repeat code - example in the section commented #binning# The code is essentially repeated with few changes between whether createCompositFile, numCPU >1 and else - have a function that defines the main section of this code and these sections become wrappers around it. We added a helper function into the main breakpointr wrapper in order to decrease unnecessary code repetition and to improve readability of the code.

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

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

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 6 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/daewoooo.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 biocLite(\"breakpointRdata\"). The package 'landing page' will be created at

https://bioconductor.org/packages/breakpointRdata

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.

mtmorgan commented 6 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/daewoooo.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 biocLite(\"breakpointR\"). The package 'landing page' will be created at

https://bioconductor.org/packages/breakpointR

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.