Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

biscuiteerData #1270

Closed
8 tasks done
jamorrison opened this issue Sep 26, 2019 · 26 comments
Closed
8 tasks done

biscuiteerData #1270

jamorrison opened this issue Sep 26, 2019 · 26 comments
Assignees
Labels
3a. accepted will be ingested into Bioconductor daily builder for distribution OK

Comments

@jamorrison
Copy link

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 understand that by submitting my package to Bioconductor,
    the package source and all review commentary are visible to the
    general public.

  • I have read the Bioconductor Package Submission
    instructions. My package is consistent with the Bioconductor
    Package Guidelines.

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

  • My package addresses statistical or bioinformatic issues related
    to the analysis and comprehension of high throughput genomic data.

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

  • The 'devel' branch for new packages and features.
  • The stable 'release' branch, made available every six
    months, for bug fixes.
  • 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
Copy link
Collaborator

Hi @jamorrison

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: biscuiteerData
Type: Package
Title: Data Package for Biscuiteer
Description: Contains default datasets used by the Bioconductor
   package biscuiteer.
Version: 0.99.0
Date: 2019-09-18
Author: Tim Triche, Jr. [aut, cre], Wanding Zhou [aut], Ben Johnson [aut],
        Jacob Morrison [aut], Lyong Heo [aut]
Maintainer: "Tim Triche, Jr." <tim.triche@gmail.com>
License: GPL-3
Depends: R (>= 3.6.0),
         ExperimentHub
Imports: AnnotationHub,
         utils,
         curl
Suggests: knitr,
biscuiteer
LazyData: true
biocViews: ExperimentHub, ExperimentData, Genome, Homo_sapiens_Data
Encoding: UTF-8
RoxygenNote: 6.1.1
Roxygen: list(markdown = TRUE)
VignetteBuilder: knitr

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
Copy link
Collaborator

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 bioc-issue-bot added 2. review in progress assign a reviewer and a more thorough review of package code and documentation taking place and removed 1. awaiting moderation labels Sep 26, 2019
@bioc-issue-bot
Copy link
Collaborator

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
Copy link
Collaborator

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

c2c0452 Fixing examples/vignettes now that data has been a...
abe9a8a Remove biscuiteer from suggests for the time being
08344e3 Merge pull request #4 from jamorrison/master Corr...

@bioc-issue-bot
Copy link
Collaborator

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
Copy link
Collaborator

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

b2c34ed Remove html vignette file for Bioconductor submiss...
5749442 Version bump for Biocondcutor
2122f44 Merge pull request #5 from jamorrison/master Bioc...

@bioc-issue-bot
Copy link
Collaborator

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.

@jamorrison
Copy link
Author

AdditionalPackage: https://github.com/trichelab/biscuiteer

@bioc-issue-bot
Copy link
Collaborator

Hi @jamorrison,

Starting build on additional package https://github.com/trichelab/biscuiteer.

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: biscuiteer
Type: Package
Title: Convenience Functions for Biscuit
Description: A test harness for bsseq loading of Biscuit output, summarization
         of WGBS data over defined regions and in mappable samples, with or
         without imputation, dropping of mostly-NA rows, age estimates, etc.
Version: 0.99.0
Date: 2019-08-22
Author: Tim Triche, Jr. [aut, cre], Wanding Zhou [aut], Ben Johnson [aut], Jacob Morrison [aut], Lyong Heo [aut]
Maintainer: "Tim Triche, Jr." <tim.triche@gmail.com>
URL: https://github.com/trichelab/biscuiteer
BugReports: https://github.com/trichelab/biscuiteer/issues
License: GPL-3
Depends: R (>= 3.6.0), 
     biscuiteerData,
     bsseq
Imports: readr,
     qualV,
     Matrix, 
     impute,
     HDF5Array,
     S4Vectors, 
     Rsamtools,
     data.table,
     Biobase,
     GenomicRanges,
     BiocGenerics,
     VariantAnnotation,
     DelayedMatrixStats,
     SummarizedExperiment,
     GenomeInfoDb,
     Mus.musculus,
     Homo.sapiens,
     matrixStats,
     QDNAseq,
     dmrseq,
     methods,
     utils,
     gtools
Suggests: DSS, 
      covr,
      knitr,
      rlang,
      scmeth,
      pkgdown, 
      roxygen2,
      testthat,
      QDNAseq.hg19,
      QDNAseq.mm10
biocViews: DataImport, MethylSeq, DNAMethylation
Encoding: UTF-8
RoxygenNote: 6.1.1
Roxygen: list(markdown = TRUE)
VignetteBuilder: knitr

@jamorrison
Copy link
Author

Mentioning @ttriche to look at package.

@bioc-issue-bot
Copy link
Collaborator

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
Copy link
Collaborator

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

a9c2ec4 Making gunzip windows compatible
81a28c1 Merge pull request #21 from jamorrison/feature_bio...

@bioc-issue-bot
Copy link
Collaborator

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.

@jamorrison
Copy link
Author

Hi @nturaga biscuiteerData and biscuiteer are ready to be reviewed. Let me know what elements I need to change. Thank you.

@lshep lshep self-assigned this Oct 7, 2019
@lshep
Copy link
Contributor

lshep commented Oct 11, 2019

Sorry for the delay - I will be taking over the review for now - I am currently looking at the packages and will post feedback within the next few hours

@lshep
Copy link
Contributor

lshep commented Oct 11, 2019

Thank you for your submission please see initial comments below:

Data package

README

  • We would recommend still showing Bioconductor installation instructions

NEWS

  • Please see ?news for formatting. Namely the section headers should be
    changes in version x.y.z and not include the package name.

DESCRIPTION

  • We recommend removing lazyData: true. We have rarely found this useful
    and actually have found it can slow installation of packages.

R/man

data.R

  • Could you include source information in the man files. Along with some
    indication of the format of the data. (ie. return type is a GRange object,
    etc)

Software package

Build report

  • Please fix global variable and function NOTES
  • Please fix coding practice NOTES

README

  • We would recommend still showing Bioconductor installation instructions

NEWS

  • Please see ?news for formatting. Namely the section headers should be
    changes in version x.y.z and not include the package name.

LICENSE

  • This file isn't needed since GPL-3 listed in description is distributed
    with base R. This should also fix the NOTE in the build report.

test

  • Remove these and replace with real tests. "tricking" the system with
    placeholders is not acceptable.

inst

  • Include an inst/script directory that describes the source information and
    any preprocessing for the data in inst/extdata

Clarification

  • You still include a lot of data with the package in data and
    inst/extdata. Is all this necessary since you uploaded data in the hub?

Vignette

  • I would suggest shorting to just biscuiteer rather than
    biscuiteer_vignette as it is much more intuitive for a user to type
    vignette("biscuiteer")

-[ ] section 2.3 while it is quick, it seems like wasted effort to recreate the
object you just did in the previous section. perhaps only create the new second
object to show combining.

  • the results of getLogitFracMeth should be a GRanges object as it expresses
    genomic positioning information. Also the idea that functions should be as
    endomorphic as possible is a standard best practice.

  • Where appropriate (meth, coefs, etc) the objects that make up the result
    list of WGBSage should be converted to GRanges.

  • While you use a show the ERROR when the results from trying to view the
    results of bisc.CpGindex should be corrected if possible as I feel most (esp. naive) users
    will try to investigate the object by the following:

> bisc.CpGindex
CpGindex with 1 row and 3 columns
    hyper.MCF7_Cunha   hypo.MCF7_Cunha ratio.MCF7_Cunha
           <numeric>         <numeric>        <numeric>
1 0.0690734126984127 0.199261516805161 0.34664702851757
  -------
This object is just a DataFrame that has an idea of where it came from:
Error in as.vector(x, mode = "character") : 
  no method for coercing this S4 class to a vector

R / man

WGVSage.R

  • As mentioned above objects in results that can be expressed as a GRanges
    object should.
  • Do /should the other parameters be checked for correct input? ie.
    stopifnot(is.logical(useENSR)) etc ?

atRegions.R

  • Output should be converted to GRanges

binCoverage.R

  • We really don't like seeing FIXME in code. These seem more like
    TODOs. (and very good TODOs)
  • Should there be parameter checking on which, QDNAseq, or readLen?

biscuitMetadata.R

  • dont call a function two different names. Choose one and export it. you
    can reference different alias in the man file without explicitly exporting
    functions of different names.

byExtremality.R

  • convert output to GRanges or GRangeList
  • remove paste0 in stop call. unnecessary

checkBiscuitBED.R

  • Might want to expand man page with details on what backing up with hdf5 or
    what a sparse matrix is. Or at least include links for more info.
  • no need to create variable err_message if it is returned right away in a
    stop. Just place right in stop call.
  • remove paste0 from message and stop calls.
  • despite having an argument for how you compute the values of colSpecs
    for readr and colClassses for data.table. Shouldn't only the specified
    how be done and stored. Otherwise whats the point of asking?

data.R

  • Expand on the documentation including source and format information.

extremality.R

  • We never really like to see FIXME in code. This bug should be fixed
    before releasing.

fexpit.R

  • Will this really be useful to a user and need to be exported?
  • Why can't this be written in one line and returned?

filterLoci.R

  • Include argument parameter checking

fixNA.R

  • sparse seems like a mis-leading entry. TRUE/FALSE both return a matrix
    object just a matter of what kind.

flogit.R

  • Is this really useful to the user to export?

getLogitFracMeth.R

  • the results of getLogitFracMeth should be a GRanges object as it expresses
    genomic positioning information. Also the idea that functions should be as
    endomorphic as possible is a standard best practice.
  • not recommended to have two functions doing the same thing. You can alias different names in the man file but dont export both.

loadHDF5.R

  • Why is this needed and not just directed to the HDF5Array function when
    necessary?

read.biscuit.R

  • Seems odd that this is the only function that has a . in the name. A dot
    typically indicates S3 dispathch which this really isn't. Would recommend
    removing and using camelcase instead.
  • don't create extra variable err_message if it is returned in the next
    line. Put the message directly into stop call.
  • not recommended to have two functions doing the same thing. choose read or
    load but don't export both. You can alias different names in the man file but
    dont export both.

saveHDF5.R

  • Why is this needed and not just directed to the HDF5Array function when
    necessary?

Please revise the packages according to above. When ready please comment here summarizing the changes. Please also remember to do a version bump to trigger a new build report and ping me in a message indicating you are ready for another review.

Cheers!

@jamorrison
Copy link
Author

Hi @lshep I wanted to let you know that I started making changes based on the corrections you requested. I will have those done early next week and answer any clarifications at that point.

@bioc-issue-bot
Copy link
Collaborator

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

1301764 Making changes for Bioconductor review process
ac3cc4c Merge pull request #6 from jamorrison/master Maki...

@bioc-issue-bot
Copy link
Collaborator

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

56103f6 First stage of Bioconductor review changes
10c6130 Changes for Bioconductor review stage 2
0efb3de Round 3 of Bioconductor review changes
5361c1a Merge pull request #23 from jamorrison/feature_bio...

@bioc-issue-bot
Copy link
Collaborator

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
Copy link
Collaborator

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.

@jamorrison
Copy link
Author

Hi @lshep I have addressed the comments you made in your review. Please see below for responses.

Data package

README

  • We would recommend still showing Bioconductor installation instructions - added

NEWS

  • Please see ?news for formatting. Namely the section headers should be
    changes in version x.y.z and not include the package name. - reformatted

DESCRIPTION

  • We recommend removing lazyData: true. We have rarely found this useful
    and actually have found it can slow installation of packages. - removed, will keep this in mind for
    future package submissions

R/man

data.R

  • Could you include source information in the man files. Along with some
    indication of the format of the data. (ie. return type is a GRange object,
    etc) - source information/return type added

Software package

Build report

  • Please fix global variable and function NOTES - fixed
  • Please fix coding practice NOTES - fixed

README

  • We would recommend still showing Bioconductor installation instructions - added

NEWS

  • Please see ?news for formatting. Namely the section headers should be
    changes in version x.y.z and not include the package name. - reformatted

LICENSE

  • This file isn't needed since GPL-3 listed in description is distributed
    with base R. This should also fix the NOTE in the build report. - removed

test

  • Remove these and replace with real tests. "tricking" the system with
    placeholders is not acceptable. - my apologies, I have added one test for the time being and will
    add more as the package matures

inst

  • Include an inst/script directory that describes the source information and
    any preprocessing for the data in inst/extdata - directory and scripts added to describe extdata, a
    script was also added to show how some of the files in the data directory were created (to go with
    the comments on data.R)

Clarification

  • You still include a lot of data with the package in data and
    inst/extdata. Is all this necessary since you uploaded data in the hub? - Some of the final
    functionality of biscuiteer is still being worked out, so we are unsure which datasets will be
    needed. Once this decision has been made, we will either remove the data entirely, or move it to
    the hub.

Vignette

  • I would suggest shorting to just biscuiteer rather than
    biscuiteer_vignette as it is much more intuitive for a user to type
    vignette("biscuiteer") - vignette name changed to biscuiteer.Rmd

  • section 2.3 while it is quick, it seems like wasted effort to recreate the object you just did in
    the previous section. perhaps only create the new second object to show combining. - removed
    repeat creation of first object

  • the results of getLogitFracMeth should be a GRanges object as it expresses
    genomic positioning information. Also the idea that functions should be as
    endomorphic as possible is a standard best practice. - getLogitFracMeth returns a GRanges now

  • Where appropriate (meth, coefs, etc) the objects that make up the result
    list of WGBSage should be converted to GRanges. - Combined meth and coefs into one GRanges
    object that is included in the output list

  • While you use a show the ERROR when the results from trying to view the
    results of bisc.CpGindex should be corrected if possible as I feel most (esp. naive) users
    will try to investigate the object by the following: - fixed

> bisc.CpGindex
CpGindex with 1 row and 3 columns
    hyper.MCF7_Cunha   hypo.MCF7_Cunha ratio.MCF7_Cunha
           <numeric>         <numeric>        <numeric>
1 0.0690734126984127 0.199261516805161 0.34664702851757
  -------
This object is just a DataFrame that has an idea of where it came from:
Error in as.vector(x, mode = "character") : 
  no method for coercing this S4 class to a vector

R / man

WGBSage.R

  • As mentioned above objects in results that can be expressed as a GRanges
    object should. - see above
  • Do /should the other parameters be checked for correct input? ie.
    stopifnot(is.logical(useENSR)) etc ? - added parameter checks

atRegions.R

  • Output should be converted to GRanges - output converted

binCoverage.R

  • We really don't like seeing FIXME in code. These seem more like
    TODOs. (and very good TODOs) - changed to TODOs
  • Should there be parameter checking on which, QDNAseq, or readLen? - added parameter
    checks

biscuitMetadata.R

  • dont call a function two different names. Choose one and export it. you
    can reference different alias in the man file without explicitly exporting
    functions of different names. - biscuitMetadata is the only exported function now

byExtremality.R

  • convert output to GRanges or GRangeList - converted
  • remove paste0 in stop call. unnecessary - removed

checkBiscuitBED.R

  • Might want to expand man page with details on what backing up with hdf5 or
    what a sparse matrix is. Or at least include links for more info. - extra description added
  • no need to create variable err_message if it is returned right away in a
    stop. Just place right in stop call. - moved string into stop call
  • remove paste0 from message and stop calls. - done
  • despite having an argument for how you compute the values of colSpecs
    for readr and colClassses for data.table. Shouldn't only the specified
    how be done and stored. Otherwise whats the point of asking? - now uses how to choose
    which values to create

data.R

  • Expand on the documentation including source and format information. - added source and
    return information

extremality.R

  • We never really like to see FIXME in code. This bug should be fixed
    before releasing. - since the DelayedArray functionality is currently experimental, I added an
    error message to cover the instance mentioned in the FIXME. When DelayedArray/HDF5-backing
    is fully implemented, this will be dealt with properly

fexpit.R

  • Will this really be useful to a user and need to be exported? - not exported anymore
  • Why can't this be written in one line and returned? - While it could be written in one line, it
    is easier to read and maintain when split over multiple lines.

filterLoci.R

  • Include argument parameter checking - included

fixNA.R

  • sparse seems like a mis-leading entry. TRUE/FALSE both return a matrix
    object just a matter of what kind. - parameter name changed to sparseMatrix

flogit.R

  • Is this really useful to the user to export? - not exported anymore

getLogitFracMeth.R

  • the results of getLogitFracMeth should be a GRanges object as it expresses
    genomic positioning information. Also the idea that functions should be as
    endomorphic as possible is a standard best practice. - see above
  • not recommended to have two functions doing the same thing. You can alias different names
    in the man file but don't export both. - only getLogitFracMeth is exported now

loadHDF5.R

  • Why is this needed and not just directed to the HDF5Array function when
    necessary? - removed

read.biscuit.R

  • Seems odd that this is the only function that has a . in the name. A dot
    typically indicates S3 dispathch which this really isn't. Would recommend
    removing and using camelcase instead. - changed to readBiscuit
  • don't create extra variable err_message if it is returned in the next
    line. Put the message directly into stop call. - moved
  • not recommended to have two functions doing the same thing. choose read or
    load but don't export both. You can alias different names in the man file but
    dont export both. - only readBiscuit is exported now

saveHDF5.R

  • Why is this needed and not just directed to the HDF5Array function when
    necessary? - removed

Let me know if you have further requests for changes to be made.

@lshep
Copy link
Contributor

lshep commented Oct 18, 2019

Thank you. I have no further comments at this time. I look forward to seeing the future developments of this package as well.
Cheers,

@lshep lshep added 3a. accepted will be ingested into Bioconductor daily builder for distribution and removed 2. review in progress assign a reviewer and a more thorough review of package code and documentation taking place labels Oct 18, 2019
@bioc-issue-bot
Copy link
Collaborator

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
Copy link
Contributor

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/jamorrison.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("biscuiteerData"). The package 'landing page' will be created at

https://bioconductor.org/packages/biscuiteerData

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
Copy link
Contributor

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/jamorrison.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("biscuiteer"). The package 'landing page' will be created at

https://bioconductor.org/packages/biscuiteer

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3a. accepted will be ingested into Bioconductor daily builder for distribution OK
Projects
None yet
Development

No branches or pull requests

5 participants