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

icetea #719

Closed
8 tasks done
vivekbhr opened this issue Apr 10, 2018 · 32 comments
Closed
8 tasks done

icetea #719

vivekbhr opened this issue Apr 10, 2018 · 32 comments
Assignees
Labels
3a. accepted will be ingested into Bioconductor daily builder for distribution ERROR

Comments

@vivekbhr
Copy link

vivekbhr commented Apr 10, 2018

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

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: icetea
Type: Package
Title: Integrating mRNA Cap Enrichment with Transcript Expression Analysis
Version: 0.99.0
Authors@R: person("Vivek", "Bhardwaj", email = "bhardwaj@ie-freiburg.mpg.de", role = c("aut", "cre"))
Description: icetea is a tool that integrates the cap enrichment information
    obtained from transcript 5' profiling methods like MAPCap and RAMPAGE,
    with the analysis of transcript expression. It provides functions for
    end-to-end analysis : from raw reads to detection of transcription
    start sites and performing differential TSS detection between group
    of samples.
Depends:
    R (>= 3.6)
Imports:
    stats,
    utils,
    methods,
    graphics,
    grDevices,
    ggplot2,
    GenomicFeatures,
    ShortRead,
    BiocParallel,
    Biostrings,
    S4Vectors,
    Rsamtools,
    BiocGenerics,
    IRanges,
    GenomicAlignments,
    GenomicRanges,
    rtracklayer,
    SummarizedExperiment,
    VariantAnnotation,
    limma,
    edgeR,
    csaw,
    TxDb.Dmelanogaster.UCSC.dm6.ensGene
Suggests:
    knitr,
    rmarkdown,
    Rsubread (>= 1.29.0),
    testthat
VignetteBuilder: knitr
biocViews: Transcription, GeneExpression, Sequencing,
    RNASeq, Transcriptomics, DifferentialExpression
URL: https://github.com/vivekbhr/icetea
BugReports: https://github.com/vivekbhr/icetea/issues
License: GPL-3 + file LICENSE
LazyData: true
RoxygenNote: 6.0.1

@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 Apr 10, 2018
@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.

@vobencha
Copy link

vobencha commented May 2, 2018

Sorry for the delay on this. I'll get you a review tomorrow.
Valerie

@vobencha
Copy link

vobencha commented May 3, 2018

Hi,

I've done a first review. Please see comments / suggestions below and let me know if you have any questions.

  1. package name

I don't see where you define what icetea stands for. While the letters may stand for 'integrating cap enrichment transcript expression analsyis' I doubt that will be immediately obvious to anyone. I would suggest putting this in 'Description' of DESCRIPTION and the vignette.

It's good you have several biocViews so at least it will be categorized and hopefully users will have an idea of what the package does.

  1. Please remove LazyData = true. This has been shown to slow down library() calls, especially when the package has large data.

  2. non-exported functions

You don't have to create man pages for functions that you do not export. If you do create them, they should be complete.

For example, ResizeReads is not exported but the man page has several problems:

\name{ResizeReads}
\alias{ResizeReads}
\title{preprocess reads to count only 5' overlaps}
\usage{
ResizeReads(reads, width = 1, fix = "start")
}
\arguments{
\item{reads}{GAlignment object}

\item{width}{New read length}

\item{fix}{'Start' for 5'}
}
\value{
Resized reads as GRanges
}
\description{
preprocess reads to count only 5' overlaps
}
  • 'width' and 'fix' should be specified as character

  • add an example

  • the description is unclear since the function simply resizes the ranges

All this function does is call GenomicRanges::resize. Do you really need a separate function for this?

 ResizeReads <- function(reads, width = 1, fix = "start") {
     reads <- as(reads, "GRanges")
     stopifnot(all(GenomicRanges::strand(reads) != "*"))
     GenomicRanges::resize(reads, width = width, fix = fix)
 }  

Please revisit the non-exported functions and decide if they are worth keeping. If they are, add detail to the man pages.

  1. newCapSet class

Why not re-use FastaFile or FastaFileList as input and SummarizedExperiment or as output? If you are set on creating your own S4 class it should extend RangedSummarizedExperiment.

  1. imports / exports

A package in 'imports' in DESCRIPTION should be fully or selectively imported in NAMESPACE. For example, these packages are in 'imports' but not mentioned in NAMESPACE.

~/repos/github/packageReview/icetea >grep ShortRead NAMESPACE
~/repos/github/packageReview/icetea >grep BiocParallel NAMESPACE
~/repos/github/packageReview/icetea >grep Biocstrings NAMESPACE

This is not a comprehensive list. Please go through DESCRIPTION and check the packages against NAMESPACE.

  1. R file names

I would recommend renaming the R files to match the function name and man page. For example, detect_TSS.R -> detectRSS.R.

~/repos/github/packageReview/icetea/R >ls
AllClasses.R         detect_diffTSS.R    get_geneCounts.R      splitBAM_byIndex.R
AllGenerics.R        detect_TSS.R        mapCaps.R             splitBAM_byRepindex.R
annotate_TSS.R       example_CSobject.R  methods.R             utils.R
demultiplex_fastq.R  filterDuplicates.R  plotting_functions.R

@vivekbhr
Copy link
Author

vivekbhr commented May 4, 2018

Thanks for the review Valerie. I would try to fix them soon

@bioc-issue-bot
Copy link
Collaborator

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

3512d0d version bump
4f7e8c3 Merge pull request #33 from vivekbhr/develop vers...

@vivekbhr
Copy link
Author

vivekbhr commented May 18, 2018

@vobencha Thanks for the review. Here are some changes I made so far :

package name

I modified the description to add what icetea stands for. (Integrating Cap Enrichment with Transcript Expression Analysis)

Please remove LazyData = true.

DONE

non-exported functions

I improved description of ResizeReads and other non-exported functions. I don't need the man pages but I would like to keep the roxygen style comments in the code so I end up generating them.

I want to keep the function ResizeReads since 1) I use this multiple times in the package. 2) we are thinking of using the package for iCLIP analysis in future, where the users would like to have the signal from the end or the middle of the reads.

newCapSet class

The purpose in my mind when creating the class was to use a container for all file paths (fastq and BAMs) along with mapping statistics, also to make it easy to check certain assumptions and fail early in the analysis.

I tried using RangedSummarizedExperiment, but I am not able to use any of its functionality. In principle, I could fit my sampleInfo DataFrame as colData and other information as metadata in the SE object. But the former throws an error (due to no assay column) and the latter requires me to skip the validity checks. Would appreciate any suggestions here.

imports / exports
I added atleast one import from each package into namespace (through roxygen directive @importfrom ), but I have still not listed all required functions there since that would mean removing all my package::function syntax from the code. I want to keep the package::function syntax in my code to : 1. avoid a function masking another 2. it's easier for me to come back after years and find which function came from which package. I could still add all imports for each of my functions through roxygen if this is absolutely required.

R file names

DONE

@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".
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 bioc-issue-bot added ERROR and removed OK labels May 18, 2018
@bioc-issue-bot
Copy link
Collaborator

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

03cd512 trying to fix bioc build check
cbc0fae version bump
777aaa1 Merge pull request #34 from vivekbhr/develop tryi...

@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".
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.

@vivekbhr
Copy link
Author

My build is passing on all platforms through appveyor and travis . Can someone point out the issue here? (I already tried listing the .gitattributes file in .Rbuildignore)

@bioc-issue-bot
Copy link
Collaborator

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

551bb72 removing .gitattributes
8d1e6da Merge pull request #35 from vivekbhr/develop remo...

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

68579bf fixed issues with bioparallel
1dbef1a adding bpisup()
fbf2a8e adding Xvector version (concatenateobjects error o...
17f3867 typo
8af0354 updating travis .yaml to fix caching issue, also u...
321b9b3 travis agai
148c386 travis again
5bd7ebf Merge pull request #36 from vivekbhr/develop fixe...

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

8d084ac removing versions: :
bda3cb4 removed versions
ed012de Merge pull request #37 from vivekbhr/develop Deve...

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

43eda77 trying to downgrate S4vectors to fix build)
e629575 change version as per travis
b1155d7 version
75b7795 Merge pull request #38 from vivekbhr/develop Deve...

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

cb0586a remove deps
2a55535 Merge pull request #39 from vivekbhr/develop remo...

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

4426e74 fix check for NA
a03c941 Merge pull request #40 from vivekbhr/develop fix ...

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

e6df250 skip suggested packages in windows build
e0680db version bump
4ec51eb Merge pull request #41 from vivekbhr/develop Deve...

@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".
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.

@vivekbhr
Copy link
Author

@vobencha I have fixed all issues with the build errors except the error with RSubread on windows. Please have a look at my replies above and let me know if additional changes are required.

Best,
Vivek

@vobencha
Copy link

Thanks for making the changes.

If you want to keep RSubread in Suggests you'll need to add a .BBSoptions file as described by Herve in
https://stat.ethz.ch/pipermail/bioc-devel/2018-May/013667.html
This file should go at the same level at R/ man/ etc.

Valerie

@vivekbhr
Copy link
Author

@vobencha It's there already. It's just not being recognized by the build system at the moment.

@vobencha
Copy link

vobencha commented Jun 1, 2018

Yes, I know the SPB does not recognize this option and the error will persist until the package is built on the formal build system. I had missed the file in the github repo when I looked for it last time. Thanks for adding it.

@vobencha vobencha 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 Jun 1, 2018
@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

mtmorgan commented Jun 2, 2018

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

https://bioconductor.org/packages/icetea

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 mtmorgan closed this as completed Jun 2, 2018
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 ERROR
Projects
None yet
Development

No branches or pull requests

4 participants