COCOA - Githubissues

j-lawson commented 6 years ago

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

Repository: https://github.com/databio/COCOA

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

Hi @j-lawson

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: PCRSA
Version: 0.99.0
Date: 2018-09-05
Title: Principal Component Region Set Analysis
Description: A method for annotating principal components of DNA methylation data with region sets. A region set is a set of genomic regions that share a biological annotation. PCRSA can identify biologically meaningful sources of variation along the PCs, increasing interpretability of the PCs and understanding of variation in the data. 
Author: John Lawson [aut, cre],
    Nathan Sheffield <http://www.databio.org> [aut]
Maintainer: John Lawson <jtl2hk@virginia.edu>
Depends: R (>= 3.5)
Imports:
    BiocGenerics,
    S4Vectors,  
    IRanges,
    GenomicRanges,
    data.table,
    ggplot2,
    Biobase,
    stats,
    methods,
    ComplexHeatmap,
    MIRA,
    tidyr,
    grid,
    grDevices
Suggests:
    knitr,
    parallel,
    testthat,
    BiocStyle,
    rmarkdown,
    AnnotationHub,
    LOLA
VignetteBuilder: knitr
License: GPL-3
biocViews: PrincipalComponent, GenomicVariation, DNAMethylation, GeneRegulation, GenomeAnnotation, 
       SystemsBiology, FunctionalGenomics, ChIPSeq, 
 MethylSeq, Sequencing, Epigenetics
URL: http://github.com/databio/PCRSA
BugReports: https://github.com/databio/PCRSA
RoxygenNote: 6.1.0

Add SSH keys to your GitHub account. SSH keys will are used to control access to accepted Bioconductor packages. See these instructions to add SSH keys to your GitHub account.

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

j-lawson commented 6 years ago

@hpages Hi Hervé,

I believe the Windows error was due to the build system and not my package. Is this correct?

Thanks, John Lawson

lshep commented 6 years ago

Sorry. Yes that ERROR was on our end. I rerunning now so you should have a full build report. @hpages will do a technical review of the package soon.

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.

j-lawson commented 6 years ago

Great! Thanks @lshep!

bioc-issue-bot commented 6 years ago

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

fbb2713 Use Authors@R: field instead of Author/Maintainer ... 526f65a Fix format 896ff17 Point vignette link to code.databio.org 593f9b3 Update IntroToPCRSA.Rmd a311bbb Added row/column titles to plots and update docs 3cd0403 Update docs and bump version b0a6d4a Merge pull request #7 from databio/dev Update plo...

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.

j-lawson commented 6 years ago

@hpages Hi Hervé,

I know this is a busy time for the Bioconductor team. I look forward to your review!

Best, John

hpages commented 6 years ago

Hi John,

Thanks for this new contribution. It looks good. There are only a few minor things that need your attention:

The titles of your man page repeat exactly the content of the Description section so are way too long. See for example ?featuresAlongPC. (I think MIRA had the same issue when I first looked at it.)

Please move GenomicRanges from Imports to Depends. This will make the basic GRanges functionality immediately available to your users. Right now, because GenomicRanges is not attached to the search path, basic functionality like the start(), end() and width() getters are not available, so your serialized GRanges objects will be perceived as broken:

library(PCRSA)
data(esr1_chr1)
esr1_chr1
# GRanges object with 765 ranges and 0 metadata columns:
#       seqnames              ranges strand
#          <Rle>           <IRanges>  <Rle>
#     1     chr1     1072710-1074806      *
#     2     chr1     1079163-1080853      *
#     3     chr1     1236995-1238060      *
#     4     chr1     1310031-1311345      *
#     5     chr1     1432920-1434006      *
#   ...      ...                 ...    ...
#   761     chr1 245046415-245047368      *
#   762     chr1 245335995-245336978      *
#   763     chr1 246583472-246584185      *
#   764     chr1 246608249-246609543      *
#   765     chr1 246632212-246633395      *
#   -------
#   seqinfo: 72 sequences from an unspecified genome; no seqlengths
start(esr1_chr1)
# [1] 1 1

Once GenomicRanges is in Depends, you don't need to prefix anything with GenomicRanges:: or to explicitely call library(GenomicRanges) in your man pages or vignettes.

Make sure that the various functions that have the mCoord argument work when a GRanges object is supplied (as advertised in the man pages):

library(PCRSA)
library(GenomicRanges)
data(brcaCoord1)
data(brcaLoadings1)
data(esr1_chr1)
data(nrf1_chr1)
GRList <- GRangesList(esr1_chr1, nrf1_chr1)
mCoord <- GRanges(brcaCoord1$chr, IRanges(brcaCoord1$start, width=1L))
pcRegionSetEnrichment(loadingMat=brcaLoadings1, mCoord=mCoord,
                      GRList=GRList,
                      PCsToAnnotate=c("PC1", "PC2"), scoringMetric="rsMean")
# Error in setnames(loadingDT, c("chr", "start", PCsToAnnotate)) : 
#  Can't assign 4 names to a 5 column data.table

Please specify the reference genome (e.g. hg19, hg38, mm10, etc...) of the various *_chr1 and brcaCoord1 datasets. Ideally the user should be able to get this information (as well as the sequence lengths and circularity flags) by calling seqinfo() on the GRanges objects.
Also it might be obvious for most users but it wouldn't hurt reminding them that all the GRanges objects that are passed to the GRList argument of pcEnrichmentProfile() and pcEnrichmentProfile() should be on the same reference genome.

Thanks, H.

j-lawson commented 6 years ago

@hpages Hi Hervé,

Thanks for the feedback! I have addressed all of your points as follows:

I shortened the function titles.
I moved GenomicRanges to "Depends".
I took out explicit calls to GenomicRanges:: and library(GenomicRanges)
I fixed the bug that prevented GRanges from being used as input and added more unit tests with GRanges input.
I added reference genome, sequence length and circularity flag info to the built in GRanges objects. I also mentioned the reference genome in the data documentation.
I mentioned that the reference genome for GRList should be consistent with the samples in the function parameter documentation. I also mentioned it in the main vignette.

In addition, I revised the main vignette/documentation and renamed the package since we think our package can have a wider scope and be applied to more data types than we initially thought. I updated the version in the Description to trigger a build but as of yet, nothing has happened. I'm not sure if there is some lag time with the system or a problem due to the name change.

Best, John

nsheff commented 6 years ago

you may need to create a new issue with the new name...

j-lawson commented 6 years ago

Since other packages were building today, I think the problem was the name. I started a new issue titled "COCOA" (which triggered a build) and will transfer the comments to that issue.

lshep commented 6 years ago

This should have stayed under this issue instead of creating a new issue - When a package name changes in the middle of the review process we need to make changes on our database side for it to register with the automatic builder

j-lawson commented 6 years ago

Thanks for the info @lshep. So should I just close the newly created issue?

nsheff commented 6 years ago

@lshep sorry, my mistake.

lshep commented 6 years ago

Yes please close the newly created issue and comment here with the changes and the information posted there - we like to keep the issues clean and all correspondence together.

lshep commented 6 years ago

The database should be updated and should pick up version bumps now. Please let us know if you have further issues.

bioc-issue-bot commented 6 years ago

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

8b8000d Update pkgdown doc 72fd5b4 Version bump for Bioc build f8c343d Version bump for Bioconductor Dev

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:

21390a8 Bump 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.

j-lawson commented 6 years ago

The warning is because the package took longer than 5 minutes to check. The first time I bumped the version number just the merida1 builder took longer but with the second version bump, both the tokay1 and merida1. I'm not sure why it took longer than 5 minutes since the almost identical version I submitted as a separate issue built on all three platforms in less than 5 minutes. Does this have to do with the build system?

bioc-issue-bot commented 5 years ago

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

a6832e8 Update Travis CI badge ce5508e Simplify some examples and take out high level uni... e86861e Add back simpler forms of unit tests and fix forma... 909515b Updates to reduce time for checking Dev

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

hpages commented 5 years ago

Hi John,

Thanks for the changes. The new name is definitely easier to remember and to pronounce than the old ones. I like it! The package is in great shape now.

Thanks for your contribution to the project.

Cheers, H.

bioc-issue-bot commented 5 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 5 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/j-lawson.keys is not empty), then no further steps are required. Otherwise, do the following:

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

https://bioconductor.org/packages/COCOA

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.

Bioconductor / Contributions

COCOA #865