OmicsLonDA - Githubissues

aametwally 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/aametwally/OmicsLonDA

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

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: OmicsLonDA
Type: Package
Title: Omics Longitudinal Differential Analysis
Version: 0.99.1
Date: 2019-03-28
Author: Ahmed A. Metwally, Tom Zhang, Michael Snyder
Maintainer: Ahmed A. Metwally <ametwall@stanford.edu>
URL: https://github.com/aametwally/OmicsLonDA
BugReports: https://github.com/aametwally/OmicsLonDA/issues
Description: Statistical method that provides robust identification of time intervals where omics features (such as proteomics, lipidomics, metabolomics, transcriptomics, the microbiome, as well as physiological parameters captured by wearable devices such as heart rhythm, body temperature, and activity level) are significantly different between groups.
License: MIT + file LICENSE
Depends:
    R(>= 3.5)
Imports:
    gss,
    plyr,
    zoo,
    pracma,
    ggplot2,
    parallel,
    doParallel,
    grDevices,
    graphics,
    stats,
    utils
Suggests:
    knitr,
    rmarkdown,
    testthat
biocViews:
    TimeCourse,
    Survival,
    Microbiome,
    Metabolomics,
    Proteomics,
    Lipidomics,
    Transcriptomics,
    Regression
Repository: CRAN
NeedsCompilation: no
Encoding: UTF-8
LazyData: true
RoxygenNote: 6.1.1
VignetteBuilder: knitr

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.

mtmorgan commented 5 years ago

If your package interoperates with rectangular (sample x feature, e.g., gene expression) data, then please make sure that main functions accept as input SummarizedExperiment, and in general re-use existing Bioconductor data representations in the interface to your package.

Your vignette must have fully evaluated code chunks (the current chunks are static 'pictures of code'). The vignette needs to contain more expository material describing the inputs, outputs, and interpretation of your method, and ideally illustrating integration with other Bioconductor packages.

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:

256464a Version bump

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.

aametwally commented 5 years ago

If your package interoperates with rectangular (sample x feature, e.g., gene expression) data, then please make sure that main functions accept as input SummarizedExperiment, and in general re-use existing Bioconductor data representations in the interface to your package.

Your vignette must have fully evaluated code chunks (the current chunks are static 'pictures of code'). The vignette needs to contain more expository material describing the inputs, outputs, and interpretation of your method, and ideally illustrating integration with other Bioconductor packages.

Thank you, Martin, for your important suggestions. We are working on enhancing the package vignette and adding SummarizedExperiment data representation as the package interface.

Best,

bioc-issue-bot commented 5 years ago

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

6f4bc3d package bump 4bd0c7d Added import of devtools in NAMESPACE ae56eb0 merge to master

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.

mtmorgan commented 5 years ago

DESCRIPTION

ok
devtools probably belongs in "Suggets:"; you do not use devtools functionality in the code in the R/ directory of your package
consider using BiocParallel instead of parallel / doParallel for interoperability with Bioconductor.
Repository: CRAN is not needed

NAMESPACE

consider selective import #' @importFrom plyr foo bar instead of importing entire packages. This makes your package more robust to changes in the packages it imports.

vignettes

emphasize installation via Bioconductor

if (!requireNamespace("BiocManager", quietly = TRUE))
    install.packages("BiocManager")
BiocManager::install("OmicsLonDA")

No need to library(devtools)
Provide a narrative description of the content of the sample data set, and the output of the omicslonda() function. Can the results be visualized in a meaningful way? How would one use the other 9 elements of diff_simulatedDataset_norm?

R (please apply these specific comments to all of your code)

omicslonda.R:53 use message() (prints to stderr) rather than cat() when communicating with your user. No need to include a trailing \n.
omicslonda.R:55 validate all input, for instance validate that required columns and data types of df are present. Use stopifnot() as a convenient way of validating input. Validate all other input as well, e.g., that fit.method is a single character string from a set of allowable values, etc.
omicslonda.R:56 all output should be, by default and in examples / vignettes, in a temporary directory so as not to over-write user files. For instance use the default prefix = tempfile() in the function signature and example.
omicslonda.R:88 complete TODO tasks before submitting a revised version.
omicslonda.R:96 it seems strange to requie a visualization step as part of a calculation function. Instead, remove this function call from the code and invoke it in examples / vignettes to illustrate relevant properties / requirements on the data.

omicslonda.R:118 please indent code following standard convention, e.g.,

model = tryCatch({
    curveFitting(formula = formula, df, method= "ssgaussian", points)
},  error = function(err) {
    print(paste("ERROR in gss = ", err, sep="")); 
    return("ERROR")
})

In the code chunk above it defeats the purpose of an error to convert the return value to a character string that the user must then check. If you would like to augment the error message with additional detail, do something like
```
model = tryCatch({
    curveFitting(formula = formula, df, method= "ssgaussian", points)
},  error = function(err) {
    stop("ERROR in gss = ", conditionMesssage(err))
})
```
omicslonda.R:126 NEVER call quit() in your code, this will end the user's R session and they will loose all their work. Instead, invoke stop() with an error
```
stop("You have entered unsupported fitting method")
```
omicslonda.R:170 seq_len(length(x)) is usually seq_along(x); here perhaps head(seq_along(x), -1)
omicslonda.R:184 is = .data a typo (no variable named .data)?
omicslonda.R:248 if the user wishes to save output.details, then it is easy enough for them to do so; no need to create this file here.
omicslondaHelper.R:306 use lapply() rather than for() to avoid the 'copy-and-append' pattern here
```
testStat.list <- lapply(perm, testStat)
```
omicslondaVisualization.R:75 and elsewhere -- consider removing this 'dead' code from your package; it is always available in the git repository, and only serves to clutter and confuse.

man

curveFitting.Rd:20 Please pay attention to describing the return value -- describe the type of data (e.g., 'a data frame with columns...') and how the user is supposed to interpret the result.

mtmorgan commented 5 years ago

Will you revise this in time for the next release?

aametwally commented 5 years ago

Yea, sure. We are working on all suggestions and planning to push our updates this Thursday.

Thanks,

bioc-issue-bot commented 5 years ago

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

774891b version bump

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:

4311622 FIxed check warnings + version bump

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:

39cdb3d Attempted to fix build warning

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.

mtmorgan commented 5 years ago

The remaining warning looks like it is not your problem. If you're ready for final review it would be great to hear a short summary of responses to the original review.

bioc-issue-bot commented 5 years ago

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

2b64636 replace parallel/doparallel with BiocParallel and ...

aametwally commented 5 years ago

Here is point by point response

devtools probably belongs in "Suggets:"; you do not use devtools functionality in the code in the R/ directory of your package

Moved it to suggests

consider using BiocParallel instead of parallel / doParallel for interoperability with Bioconductor.

Done

Repository: CRAN is not needed

Removed

consider selective import #' @importFrom plyr foo bar instead of importing entire packages. This makes your package more robust to changes in the packages it imports.

Used importFrom instead of import as much we could

emphasize installation via Bioconductor if (!requireNamespace("BiocManager", quietly = TRUE)) install.packages("BiocManager") BiocManager::install("OmicsLonDA")

Added

No need to library(devtools)

Removed

Provide a narrative description of the content of the sample data set, and the output of the omicslonda() function. Can the results be visualized in a meaningful way? How would one use the other 9 elements of diff_simulatedDataset_norm?

Added a clear description about the input data to the vignette

omicslonda.R:53 use message() (prints to stderr) rather than cat() when communicating with your user. No need to include a trailing \n.

Done

omicslonda.R:55 validate all input, for instance validate that required columns and data types of dfare present. Use stopifnot() as a convenient way of validating input. Validate all other input as well, e.g., that fit.method is a single character string from a set of allowable values, etc.

Done

omicslonda.R:56 all output should be, by default and in examples / vignettes, in a temporary directory so as not to over-write user files. For instance use the default prefix = tempfile() in the function signature and example.

Done

omicslonda.R:88 complete TODO tasks before submitting a revised version.

Done

omicslonda.R:96 it seems strange to requie a visualization step as part of a calculation function. Instead, remove this function call from the code and invoke it in examples / vignettes to illustrate relevant properties / requirements on the data.

Agree. We moved all visualization methods to outside omicslonda. Now, the user can call each from the input data and the returned output of omicslonda

omicslonda.R:118 please indent code following standard convention, e.g., model = tryCatch({ curveFitting(formula = formula, df, method= "ssgaussian", points) }, error = function(err) { print(paste("ERROR in gss = ", err, sep="")); return("ERROR") })

Done

In the code chunk above it defeats the purpose of an error to convert the return value to a character string that the user must then check. If you would like to augment the error message with additional detail, do something like model = tryCatch({ curveFitting(formula = formula, df, method= "ssgaussian", points) }, error = function(err) { stop("ERROR in gss = ", conditionMesssage(err)) })

Done

omicslonda.R:126 NEVER call quit() in your code, this will end the user's R session and they will loose all their work. Instead, invoke stop() with an error stop("You have entered unsupported fitting method")

Done

omicslonda.R:170 seq_len(length(x)) is usually seq_along(x); here perhaps head(seq_along(x), -1)

Done

omicslonda.R:184 is = .data a typo (no variable named .data)?

We used .data to overcome the unbounded variable CRAN check of “no visible global variable”

omicslonda.R:248 if the user wishes to save output.details, then it is easy enough for them to do so; no need to create this file here.

Done. We removed those saving commands from the omicslonda method. We give an example in the vignette on how user can save the output.

omicslondaHelper.R:306 use lapply() rather than for() to avoid the 'copy-and-append' patternhere testStat.list <- lapply(perm, testStat)

Done

omicslondaVisualization.R:75 and elsewhere -- consider removing this 'dead' code from your package; it is always available in the git repository, and only serves to clutter and confuse.

Done

curveFitting.Rd:20 Please pay attention to describing the return value -- describe the type of data (e.g., 'a data frame with columns...') and how the user is supposed to interpret the result.

Done

aametwally commented 5 years ago

If your package interoperates with rectangular (sample x feature, e.g., gene expression) data, then please make sure that main functions accept as input SummarizedExperiment, and in general re-use existing Bioconductor data representations in the interface to your package.

Now, omicslonda takes its input as SummarizedExperiment object

Your vignette must have fully evaluated code chunks (the current chunks are static 'pictures of code'). The vignette needs to contain more expository material describing the inputs, outputs, and interpretation of your method, and ideally illustrating integration with other Bioconductor packages.

Done

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.

aametwally commented 5 years ago

Hi @mtmorgan

Currently, I have this warning:

'library' or 'require' call not declared from: 'gss' 'library' or 'require' call to 'gss' in package code. Please use :: or requireNamespace() instead. See section 'Suggested packages' in the 'Writing R Extensions' manual.

This is because I need to load gss library to be used on each worker when using BiocParallel::bplapply. I'd appreciate if you let me know how to fix this warning.

Thanks,

bioc-issue-bot commented 5 years ago

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

cfa6b88 Fixed warning + version bump

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.

zhangtom54321 commented 5 years ago

Hi @mtmorgan

Currently, I have this warning:

'library' or 'require' call not declared from: 'gss' 'library' or 'require' call to 'gss' in package code. Please use :: or requireNamespace() instead. See section 'Suggested packages' in the 'Writing R Extensions' manual.

This is because I need to load gss library to be used on each worker when using BiocParallel::bplapply. I'd appreciate if you let me know how to fix this warning.

Thanks,

All good, it's fixed now.

mtmorgan commented 5 years ago

Can you increment the version of your package to trigger a build?

bioc-issue-bot commented 5 years ago

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

f8e4045 Version bump

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.

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

https://bioconductor.org/packages/OmicsLonDA

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.

aametwally commented 5 years ago

Thank you, Martin!

Bioconductor / Contributions

OmicsLonDA #1070