Bioconductor / Contributions

Contribute Packages to Bioconductor
135 stars 33 forks source link

TimeSeriesExperiment #904

Closed nlhuong closed 6 years ago

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

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: vistimeseq
Type: Package
Title: Analysis for short time-course sequencing data
Version: 0.99.0
Date: 2018-09-30
Author: Lan Huong Nguyen
Maintainer: Lan Huong Nguyen <nlhuong90@gmail.com>
Description: Visualization and analysis toolbox for short time course data 
  which includes dimensionality reduction, clustering, two-sample differential 
  expression testing and gene ranking techniques. The package also provides 
  methods for retrieving enriched pathways.
License: LGPL (>= 3)
Depends:
  R (>= 3.5)
VignetteBuilder: knitr
Imports:
    Biobase,
    circlize,
    dynamicTreeCut,
    dplyr,
    edgeR,
    DESeq2,
    ggplot2,
    graphics,
    grDevices,
    grid,
    Hmisc,
    limma,
    methods,
    magrittr,
    proxy,
    RColorBrewer,
    stats,
    tibble,
    tidyr,
    vegan,
    viridis,
    utils
Suggests:
    knitr,
    GO.db,
    org.Mm.eg.db,
    org.Hs.eg.db,
    MASS,
    rmarkdown,
    ComplexHeatmap,
    UpSetR,
    testthat
Encoding: UTF-8
RoxygenNote: 6.1.0.9000
biocViews: TimeCourse, Sequencing, RNASeq, Microbiome,
    GeneExpression, Transcription, Normalization, DifferentialExpression,
    PrincipalComponent, Clustering, Visualization, Pathways
BugReports: https://github.com/nlhuong/vistimeseq/issues
URL: https://github.com/nlhuong/vistimeseqv
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:

f4d035f bump up version for Bioconductor new build 21d9c94 add importfrom methods for function is() 447b4c3 fix documentation and bugs

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:

9a81095 bump the version up

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.

mtmorgan commented 6 years ago

DESCRIPTION / etc

vignette

R

tests

NEWS

nlhuong commented 6 years ago

Hi,

I am working currently on implementing your suggestions, and have the following questions so far.

  1. SummarizedExperiment: I will add a functionality to convert between SummarizedExperiment and vistimeseq objects. However, I did not decide on building vistimeseq by extending the SummarizedExperiment class, due to differences in the focus. vistimeseq should be applicable to a wide variety of rectangular data where samples have a corresponding time. I did not want all the utilities offered by SummarizedExperiment to confuse the user.

Thus, I think re-writing the class as a SummarizedExperiment extensions would require a lot of work, with little advantage, and result in additional weaknesses of for the package.

  1. BioFileCache seems to be buggy. I installed the package from the Bioconductor and call:
library(BiocFileCache)
url1 <- paste0("https://www.ncbi.nlm.nih.gov/geo/download/?acc=GSE114762&",
              "format=file&file=GSE114762_raw_counts.csv.gz")
cnts <- read_csv(bfcrpath(BiocFileCache(ask = FALSE), rnames = url1))

which which results in an error: Error in vapply(rnames, function(bfc, rname) { : values must be length 1, but FUN(X[[1]]) result is length 0

Do you have any recommendations as to how to resolve this?

Thanks!

mtmorgan commented 6 years ago

I do not insist that you use SummarizedExperiment, but feel that extending well-tested code i s likely to result in more robust and interoperable software than ad hoc class implementation. Stylistically, Bioconductor users expect CamelCase class names and camelCase function names.

I see the error you mention when using the release version of BiocFileCache, but not the devel version. Are you using the devel version of Bioconductor packages to test your code? This is where your package will be introduced to Bioconductor. I think your cache is corrupt (multiple resources with identical rname) and needs to be cleaned up

bcfquery(BiocFileCache(), rname=url1, exact = TRUE)  # several resources
bfcremove(BiocFileCache(), rids = paste0("BFC", 1:3))  # e.g.

For version I have

> sessionInfo()
R version 3.5.1 Patched (2018-10-05 r75405)
Platform: x86_64-apple-darwin17.7.0 (64-bit)
Running under: macOS High Sierra 10.13.6

Matrix products: default
BLAS: /Users/ma38727/bin/R-3-5-branch/lib/libRblas.dylib
LAPACK: /Users/ma38727/bin/R-3-5-branch/lib/libRlapack.dylib

locale:
[1] en_US.UTF-8/en_US.UTF-8/en_US.UTF-8/C/en_US.UTF-8/en_US.UTF-8

attached base packages:
[1] stats     graphics  grDevices utils     datasets  methods   base

other attached packages:
[1] BiocFileCache_1.5.7 dbplyr_1.2.2

loaded via a namespace (and not attached):
 [1] Rcpp_0.12.19     digest_0.6.18    crayon_1.3.4     dplyr_0.7.7
 [5] assertthat_0.2.0 rappdirs_0.3.1   R6_2.3.0         DBI_1.0.0
 [9] magrittr_1.5     RSQLite_2.1.1    httr_1.3.1       pillar_1.3.0
[13] rlang_0.2.2      blob_1.1.1       bindrcpp_0.2.2   tools_3.5.1
[17] bit64_0.9-7      glue_1.3.0       bit_1.1-14       purrr_0.2.5
[21] compiler_3.5.1   pkgconfig_2.0.2  memoise_1.1.0    bindr_0.1.1
[25] tidyselect_0.2.5 tibble_1.4.2
bioc-issue-bot commented 6 years ago

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

b4654ea bump up version and edit travis

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

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

05df5df 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, 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.

nlhuong commented 6 years ago

It seems that the BiocFileCache fails for Windows but not other platforms.

Also, do you have any recommendations for speeding up the check part? My package exceeds the limit time my around 1 second.

mtmorgan commented 6 years ago

thank you for the changes, I will review them today. The Windows problem will be addressed through a fix to BiocFileCache; do not worry about the Mac timing.

mtmorgan commented 6 years ago

vignettes

NEWS

bioc-issue-bot commented 6 years ago

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

5245540 fix typos and NEWS

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.

nlhuong commented 6 years ago

Hi,

I have thought about your comments about extending SummarizedExperiment class and decided to change my package.

I have pushed my changes to vistimeseq/extendSE branch but now I would like to name my package TimeSeriesExperiment which I think is more in line with the Bioconductor conventions.

I have a few WARNINGS I do not know how to resolve to the new package based on extending SummarizedExperiment class:

❯ checking for missing documentation entries ... WARNING
  Undocumented S4 methods:
    generic 'assays' and siglist 'TimeSeriesExperiment'
    generic 'colData' and siglist 'TimeSeriesExperiment'
    generic 'colnames<-' and siglist 'TimeSeriesExperiment'
    generic 'rowData' and siglist 'TimeSeriesExperiment'
    generic 'rownames<-' and siglist 'TimeSeriesExperiment'
  All user-level objects in a package (including S4 classes and methods)
  should have documentation entries.
  See chapter ‘Writing R documentation files’ in the ‘Writing R
  Extensions’ manual.

❯ checking Rd \usage sections ... WARNING
  Undocumented arguments in documentation object 'assays'
    ‘withDimnames’
  Objects in \usage without \alias in documentation object 'assays':
    ‘\S4method{assays}{TimeSeriesExperiment}’

  Objects in \usage without \alias in documentation object 'colData':
    ‘\S4method{colData}{TimeSeriesExperiment}’

  Objects in \usage without \alias in documentation object 'colnames':
    ‘\S4method{colnames<-}{TimeSeriesExperiment}’
    ‘\S4method{rownames<-}{TimeSeriesExperiment}’

  Undocumented arguments in documentation object 'rowData'
    ‘use.names’
  Objects in \usage without \alias in documentation object 'rowData':
    ‘\S4method{rowData}{TimeSeriesExperiment}’

  Functions with \usage entries need to have the appropriate \alias
  entries, and all their arguments documented.
  The \usage entries must correspond to syntactically valid R code.
  See chapter ‘Writing R documentation files’ in the ‘Writing R
  Extensions’ manual.

❯ checking dependencies in R code ... NOTE
  Unexported object imported by a ':::' call: ‘S4Vectors:::selectSome’
    See the note in ?`:::` about the use of this operator.
  1. Could you advise me on how to get rid of these warnings?
  2. How should I update the changes to the package for this issue (including the change of the package name)? What should I keep increasing the version number > 0.99.5 or reset?
  3. Will we be able to make changes before the deadline for Bioconductor 3.8?

Thank you, Lan

mtmorgan commented 6 years ago

Since your class 'contains' SummarizedExperiment, it is not necessary to define methods assay() etc; the existing methods just 'work'. You still want to import the generic or importMethods the methods.

Obviously your user would like to work with your objects using the SummarizedExperiment interface; it makes sense to put SummarizedExperiment in the Depends: field. If instead you leave as an import, and then re-export specific methods, then yes it is necessary to re-document these, probably on a page that says 'see the SummarizedExperiment documentation'. But this seems like extra work and confusing to your user; it is better to put SummarizedExperiment in Depends:

The "objects in \usage without \alias" come from documentation where the \usage{} section references a method that is not in the \alias{} section, for instance man/assay.Rd has

\S4method{assays}{TimeSeriesExperiment}(x, ..., withDimnames = TRUE)

and so needs

\usage{assays,TimeSeriesExperiment-method}

Currently \usage{assays} says that you're going to document the generic, but actually you don't and this should be removed.

Both of these issues seem to come from the use of the roxygen tag @name; remove it and the documentation looks better. I don't know if this is a bug or somehow 'feature'. I think also you can add a specific @aliases assays,TimeSeriesExperiment-method.

Continue to make version bumps. Rename the repository to TimeSeriesExperiment. Update (edit the comment) the url in the first comment on this issue to point to the correct repository. Let me know when you are ready, and we'll adjust things on our end; you'll then have to do a version bump to trigger a new package build.

mtmorgan commented 6 years ago

We will try to get the package into the 3.8 release; the sooner the changes are complete the more likely we are for success...

nlhuong commented 6 years ago

Thank you for your help!

I have changed the dependencies and now TimeSeriesExperiment depends on SummarizedExperiment. As I understand, I do not have to document the methods. I do need to re-write a few setter methods though, so that they affect new slots of my class. I still get the warnings about the generics. Even with the examples like the following:

#' @importMethodsFrom SummarizedExperiment "rowData<-"
#' @export
setReplaceMethod("rowData", "TimeSeriesExperiment", function(x, ..., value) {
    if(nrow(value) != nrow(x))
        stop("nrow(value) does not match the input object dimensions.")
    if(!"feature" %in% base::colnames(value)) {
        stop("rowData data must contain columns 'feature'.")
    }
    if(!all(value$feature == rownames(x))) {
      rownames(x) <- value$feature
    }
    newobject <- callNextMethod()
    validObject(newobject)
    return(newobject)
})

#' @importFrom methods slot slot<-
#' @export
setReplaceMethod(
  "colnames", "TimeSeriesExperiment", function(x, value) {
    if(length(value) != ncol(x))
      stop("Wrong length of substitute vector for colnames.")
    newobject <- x
    colnames(newobject) <- value
    # Additionally modify the dimensionRediuction slot
    dimreds <- slot(newobject, "dimensionReduction")
    if(is.null(names(dimreds))) return(newobject)
    for (name_mat in names(dimreds)[grep("_sample", names(dimreds))]) {
        base::rownames(dimreds[[name_mat]]) <- value
    }
    slot(newobject, "dimensionReduction") <- dimreds
    validObject(newobject)
    return(newobject)
  }
)

How do I import the generics from SummarizedExperiment so that the warning goes away?

Thank you!

mtmorgan commented 6 years ago

Can you be specific about the warnings that you get? If you define a method, then you DO need to document the new method; if there are no methods defined but existing methods are useful to the user, they do NOT need to be documented by you.

bioc-issue-bot commented 6 years ago

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

1a239e6 changing the class to summarizedexperiment extensi... 6cd44f0 changing the class to summarizedexperiment extensi... 0604ac6 Change vistimeseq class to TimeSeriesExperiment, a...

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.

nlhuong commented 6 years ago

Hi @lshep , I am changing the name for the package and have renamed the repository and edited the comment for this issue. Should I now edit the issue title? Could you sync the issue? Thanks, Lan

lshep commented 6 years ago

I have updated the issue in our database. Yes please change the issue title and you should be all set. Let us know if you run into any issues.

nlhuong commented 6 years ago

Thank you!

bioc-issue-bot commented 6 years ago

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

85affcd fix class methods

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:

7a97022 fix documentation

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:

35193c2 fix documentation for windows

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.

nlhuong commented 6 years ago

Hi, the package now has no warnings and errors. Please let me know if you can review it, as it seems today is the deadline for commits for 3.8 version.

bioc-issue-bot commented 6 years ago

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

d554092 update NEWS 49b1fce 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.

Congratulations! The package built without errors or warnings on all platforms.

Please see the build report for more details.

nlhuong commented 6 years ago

Hi @mtmorgan!

My package passes the build and checks and is ready for re-review. Please see if it is ready to be accepted.

Thank you! Lan

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

Thanks for your work on this; I think the package has been improved!

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

https://bioconductor.org/packages/TimeSeriesExperiment

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.