(inactive) TeMPO - Githubissues

MalteThodberg commented 5 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/MalteThodberg/TeMPO

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 5 years ago

Hi @MalteThodberg

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: TeMPO
Type: Package
Title: Tidy Meta Profiles using Bioconductor and the Tidyverse
Version: 0.2.0
Author: Malte Thodberg
Maintainer: Malte Thodberg <maltethodberg@gmail.com>
Description: Calculate the meta-profile across multiple sets, strands and signals using Bioconductor and output the result in tidy format for further analysis and plotting using the tidyverse. Includes advanced functionality for outlier trimming, custom summary functions and parallel execution.
Depends:
    R (>= 3.5),
    S4Vectors,
    GenomicRanges,
    rtracklayer
Imports:
    assertthat,
    methods,
    BiocGenerics,
    IRanges,
    GenomeInfoDb,
    BiocParallel,
    matrixStats,
    tibble,
    dplyr
Suggests:
    knitr,
    rmarkdown,
    BiocStyle,
    AnnotationHub,
    tidyr,
    ggplot2,
    tidyverse
License: GPL-3 + file LICENSE
Encoding: UTF-8
LazyData: true
RoxygenNote: 6.1.0
VignetteBuilder: knitr
Collate: 
    'aaa.R'
    'AllClasses.R'
    'AllGenerics.R'
    'agnosticImport-methods.R'
    'data.R'
    'quantileTrim-methods.R'
    'tidyMetaProfile-methods.R'
    'wideMetaProfile-methods.R'
biocViews: Software, Coverage, DataImport, DataRepresentation, Preprocessing, Visualization, Transcriptomics, Epigenetics, NucleosomePositioning, HistoneModification

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

On one or more platforms, the build results were: "skipped, TIMEOUT". 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 5 years ago

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

1b52239 removed dependance on AnnotationHub and instead ad... 1b26045 Merge branch 'master' of github.com:MalteThodberg/...

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.

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.

lshep commented 5 years ago

Please fix all ERROR's before a technical review will be given.

bioc-issue-bot commented 5 years ago

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

1d2fae4 Reduced size of example files.

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.

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 5 years ago

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

60f9429 Further reduced sizes

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.

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 5 years ago

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

94c8bda Reduced file sizes even further

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.

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 5 years ago

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

dab8151 Reduced size by removing TSSs

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.

Kayla-Morrell commented 5 years ago

Hi @MalteThodberg,

Thank you for submitting to Bioconductor. Please see the initial review of your package below:

DESCRIPTION Overall the file looks good and these are my comments:

[ ] SUGGESTION: it's not necessary to include the + file LICENSE since the GNU General Public License comes standard with R.
[ ] REQUIRED: LazyData: false, this will help speed up the loading of the package.

NAMESPACE Looks good, nothing to add or change.

Vignette The vignette looks good as a whole, the abstract looks nice and there is a good explanation of what TeMPO needs for input and what it outputs. These are the comments I had:

[ ] REQUIRED: .R and .html files are not needed and should be removed.
[ ] SUGGESTION: The name of the vignette is currently TemPO, I'm not sure if this was on purpose or not but it might be helpful to keep the vignette name the same as the package name. I feel that users will automatically try to look for the vignette using the package name.
[ ] SUGGESTION: For the #NOTE: in your first AnnotationHub() example, I would consider trying to break it up over a few lines. It's the only chunk of code that the user has to scroll to read the whole note.
[ ] REQUIRED: AnnotationHub() should be loaded as well. I know eval=FALSE for these sections but if the user wants to run the code they have to load it themselves.

I went through and ran the vignette code on my own computer, from start to finish. There were two sections where I seemed to run into a problem on my machine. Though everything still renders in your vignette I just wanted to bring them to your attention incase there are problems in the future from other users.

Single set of sites & multiple genome-wide signal
- I am able to execute the code to define the SM variable but when I plot it I had 4 lines even though your vignette example has 3.
- The plot also causes my R to freeze and I have to quit/restart R.
Meta-Profile showing the difference in epigenetic modifications at TSSs vs enhancers plot
- I ran this code twice, once the plot of MM1 froze my R session and the second time the code timed out all together.

R files All your code looks well formatted and documented. I had one comment:

[ ] SUGGESTION: The aaa.R file isn't totally necessary. These imports could be loaded in at the AllClasses.R or AllGenerics.R files. It doesn't seem necessary to just have an empty file for just imports.

man files Overall all the files look good, there is some great documentation and explanations throughout. Below are my comments:

[ ] SUGGESTION: agnosticImport.Rd has the same description as the title. It might be helpful to give a bit more information here, what is being done and why, why might a user use this.
[ ] SUGGESTION: agnosticImport.Rd could use more explanation for the arguments.
[ ] REQUIRED: agnosticImport.Rd the example for BigWig isn't shown, I think you are commenting out the \donotrun{} with the double # in AllGenerics.R.
[ ] SUGGESTION: CAGE_clusters.Rd the keyword doesn't seem to show, its should be plural \keywords.
[ ] SUGGESTION: quantileTrim.Rd has the same description as the title, could use more of an explanation of what is being done and why, why might a user use this.
[ ] SUGGESTION: quantileTrim.Rd the arguments could use more of a description.

Data Well documented data, nothing more to say for this section.

General comments Everything looked well documented and formatted through out. Your code looks very clean.

[ ] SUGGESTION: I would highly recommend adding in some test functions for your code. Since you used similar code chunks through your code, especially in tidyMetaProfile-methods.R and wideMetaProfile-methods.R, tests could help catch any changes that might have been made in one area but wasn't carried through out the entire code.

Comment back here with updates that have been made and when the package is ready for a re-review.

Best, Kayla

Kayla-Morrell commented 5 years ago

Hello @MalteThodberg, Just checking in, have you made any progress or have any questions about updates? We normally like to see some progress within 2-4 weeks of sending feedback to know the issue is still open and active.

MalteThodberg commented 5 years ago

Hello Kayla,

I'm working on it, although I haven't pushed any changes to the GitHub just yet.

One general question:

Is it possible to use data from AnnotationHub in a vignette? E.g. use a BigWig-file via AnnotationHub instead of storing a subset of it in the R-package.

I tried using AnnotationHub in the vignette, which works fined locally (I guess since the BigWig is cached) but the long download time causes the vignette check to fail when checking on Bioconductor.

Kayla-Morrell commented 5 years ago

Hello,

It is possible to use data from AnnotationHub in a vignette. The initial build may throw errors/warnings because of the long download time, but once it's cached the remaining builds should be fine. If you want to do this and run into issues along the way we can do somethings on our end to help.

-Kayla

MalteThodberg commented 5 years ago

That would be great - even the subset of BigWigFiles are quite large to store in extdata, pushing the package size towards the limit. It would be much easier to just use Annotation hub to load the needed BigWigs ("AH32877", "AH32879", "AH32881", "AH32884").

What should I do on my side to make it work using AnnotationHub?

Kayla-Morrell commented 5 years ago

Is this your own data or data that is already in AnnotationHub?

@lshep should be able to help more with this as well.

lshep commented 5 years ago

You should be able to use the AnnotationHub code directly in your package to download/load the files of interest. The first run may take awhile and cause WARNINGS/TIMEOUTS because of the download time, but subsequent runs should access the cached file and load quickly. It is advisable to use annotationhub resources over providing your own so we would recommend removing these from extdata. Please see removing data - realize you may have to change the clean file size as the commands shown use 100M as the clean size - but a smaller designation is probably more appropriate.

lshep commented 5 years ago

From the looks internally - it uses rtracklayer to download and load the file as a BigWigFile object which than can be passed to the rtracklayer::import method to load as a GRanges object The rtracklayer manual may be useful. so something like

> bigWigFile = ah[["AH32877"]]
> bigWigFile
BigWigFile object
resource: /home/lori//.AnnotationHub/38317 

> bigWig = rtracklayer::import(bigWigFile)
> bigWig
GRanges object with 86724789 ranges and 1 metadata column:
             seqnames              ranges strand |            score
                <Rle>           <IRanges>  <Rle> |        <numeric>
         [1]     chr1             1-10085      * |                0
         [2]     chr1         10086-10090      * | 1.02402997016907
         [3]     chr1         10091-10092      * | 2.04833006858826
         [4]     chr1         10093-10149      * |   3.072350025177
         [5]     chr1         10150-10178      * | 4.09665012359619
         ...      ...                 ...    ... .              ...
  [86724785]     chrX           155260309      * | 5.12067985534668
  [86724786]     chrX 155260310-155260351      * | 4.09665012359619
  [86724787]     chrX 155260352-155260363      * |   3.072350025177
  [86724788]     chrX 155260364-155260368      * | 2.04833006858826
  [86724789]     chrX           155260369      * | 1.02402997016907
  -------
  seqinfo: 24 sequences from an unspecified genome

MalteThodberg commented 5 years ago

Thanks for the response.

Let me see if I understand you correctly I should:

1) Change code to use AnnotationHub. 2) Push changes to GitHub. 3) Ignore TIMEOUT warnings? 4) Push new changes, and this time the AnnotationHub records should be cached?

Ideally I would like to use 4 different BigWigs from AnnotationHub ("AH32877", "AH32879", "AH32881", "AH32884"), so it might take quite a while to cache them all on the first run...

lshep commented 5 years ago

Yes that is correct. Some of those files are already cached on the builders so it might not be as bad as you think. Cheers.

bioc-issue-bot commented 5 years ago

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

00ebdf3 Bioconductor review changes

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.

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 5 years ago

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

1ea113a Added unit tests and more checks + README

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.

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 5 years ago

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

3153796 Formatted README and NEWS. Skipping BigWig checks ...

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.

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.

Kayla-Morrell commented 5 years ago

Hello @MalteThodberg,

Just wondering if you are ready for a re-review or if you are still making changes/edits to your package?

Thanks, Kayla

MalteThodberg commented 5 years ago

I'm still making some small changes based on user input, plus fixing the last of the review points

Kayla-Morrell commented 5 years ago

I am closing this package for inactivity. We normally like to see changes to a package within a few weeks to know they are actively being worked on (no changes have been committed in over a month). When you have implemented changes and are ready for a re-review, please comment back here to ask that the issue be reopened.

Best, Kayla

bioc-issue-bot commented 5 years ago

This issue is being closed because there has been no progress for an extended period of time. You may reopen the issue when you have the time to actively participate in the review / submission process. Please also keep in mind that a package accepted to Bioconductor requires a commitment on your part to ongoing maintenance.

Thank you for interest in Bioconductor.

MalteThodberg commented 5 years ago

Yes, sorry for the delay, I was pondering making some very significant changes that would affect the overall package setup. I will reopen as soon as it's settled.

Bioconductor / Contributions

(inactive) TeMPO #907