ShinyÉPICo - Githubissues

omorante commented 3 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/omorante/shinyepico

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.
[x] I am familiar with the Bioconductor code of conduct and agree to abide by it.

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

Hi @omorante

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: shinyepico
Title: ShinyÉPICo
Version: 0.99.0
Authors@R: person('Octavio', 'Morante-Palacios', email = 'octaviompa@gmail.com', role = c('cre', 'aut'))
Description: ShinyÉPICo is a graphical pipeline to analyze Illumina DNA methylation arrays (450k or EPIC). It allows to calculate differentially methylated positions and differentially methylated regions in a user-friendly interface. Moreover, it includes several options to export the results.
License: AGPL-3 + file LICENSE
Depends: R (>= 4.0.0)
Imports: 
    DT (>= 0.15.0),
    IlluminaHumanMethylation450kanno.ilmn12.hg19,
    IlluminaHumanMethylation450kmanifest,
    IlluminaHumanMethylationEPICanno.ilm10b4.hg19,
    IlluminaHumanMethylationEPICmanifest,
    data.table (>= 1.13.0),
    doParallel (>= 1.0.0),
    dplyr (>= 1.0.0),
    foreach (>= 1.5.0),
    GenomicRanges (>= 1.38.0),
    ggplot2 (>= 3.3.0),
    gplots (>= 3.0.0),
    heatmaply (>= 1.1.0),
    limma (>= 3.42.0),
    minfi (>= 1.32.0),
    plotly (>= 4.9.2),
    reshape2 (>= 1.4.0),
    rlang (>= 0.4.0),
    rmarkdown (>= 2.3.0),
    rtracklayer (>= 1.46.0),
    shiny (>= 1.5.0),
    shinyWidgets (>= 0.5.0),
    shinycssloaders (>= 0.3.0),
    shinyjs (>= 1.1.0),
    shinythemes (>= 1.1.0),
    statmod (>= 1.4.0),
    tidyr (>= 1.1.0),
    zip (>= 2.1.0)
Suggests:
    knitr (>= 1.30.0),
    mCSEA (>= 1.10.0)
Encoding: UTF-8
LazyData: true
RoxygenNote: 7.1.1
URL: https://github.com/omorante/shiny_epico
BugReports: https://github.com/omorante/shiny_epico/issues 
VignetteBuilder: knitr
biocViews: DifferentialMethylation,DNAMethylation,Microarray,Preprocessing,QualityControl

bioc-issue-bot commented 3 years ago

A reviewer has been assigned to your package. Learn what to expect during the review process.

IMPORTANT: Please read this documentation for setting up remotes to push to git.bioconductor.org. It is required to push a version bump to git.bioconductor.org to trigger a new build.

Bioconductor utilized your github ssh-keys for git.bioconductor.org access. To manage keys and future access you may want to active your Bioconductor Git Credentials Account

bioc-issue-bot commented 3 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, 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. This link will be active for 21 days.

Remember: if you submitted your package after July 7th, 2020, when making changes to your repository push to git@git.bioconductor.org:packages/shinyepico to trigger a new build. A quick tutorial for setting up remotes and pushing to upstream can be found here.

bioc-issue-bot commented 3 years ago

Received a valid push on git.bioconductor.org; starting a build for commit id: 82ee15bbfca1058d792d9afbe25cb63d29d4f219

bioc-issue-bot commented 3 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. This link will be active for 21 days.

Remember: if you submitted your package after July 7th, 2020, when making changes to your repository push to git@git.bioconductor.org:packages/shinyepico to trigger a new build. A quick tutorial for setting up remotes and pushing to upstream can be found here.

omorante commented 3 years ago

Hi @mtmorgan,

Since the two warnings that my package produce are the same that I had read before on my computer, I would like to explain why I can not fix them (or I do not know the best way to do it) at this point.

WARNING: Import IlluminaHumanMethylation450kanno.ilmn12.hg19, IlluminaHumanMethylation450kmanifest, IlluminaHumanMethylationEPICanno.ilm10b4.hg19, IlluminaHumanMethylationEPICmanifest, statmod in NAMESPACE as well as DESCRIPTION.

My package depends of some packages that are needed at some point in the analysis (for example, Illumina450k or IlluminaEPIC annotations are used by minfi). However, I do not use them explicitly, because minfi package loads the annotation that it needs depending on the data.

* WARNING: Remove set.seed usage in R code
  Found in R/ directory functions:
    run_shinyepico()

My package also uses another package, mCSEA, to perform DMR calculation. It uses permutations to calculate p values, and without a seed the results are not reproducible. I have incorporated an option to the function that loads the shiny app, that allows the user to set a seed, or not. Maybe, if you think that this is not a good practice, I could explain this in the vignette and let the users to set a seed by themselves. However, in my opinion this could be a bit less practical (if, for example, the user sets a seed, perform the analysis and do not remember the seed in the future). (One of the targets of my application are users with not much previous experience with R.)

But of course I am open to discuss and implement solutions to these and other issues that you could find in the package!

mtmorgan commented 3 years ago

set.seed() -- make the default behavior that the seed is not set (the user has to explicitly use your argument seed = ...). This requires the user to think about reproducibility versus uncertainty. It is also just a small step from this to an instruction in the vignette 'for reproducibility, call set.seed(...) before ...', and then we are at the original recommendation, to leave the random number seed alone.

For IlluminaHumanMethylation450kanno.ilmn12.hg19 and friends, it looks like these are used in the vignette, and so should be in the Suggests: rather than Imports: field. They will be installed when the Bioconductor build system builds the vignette, but not (by default) when the user installs your package. I am interpreting your use to be that some of these packages, or perhaps others, will be used, but not necessarily all of these packages. If you mean that these packages have to be installed for all users of your package, then they should be listed in the Depends: field.

omorante commented 3 years ago

Thank you for your advice!

Regarding set.seed, I can do that, make the seed "NULL" by default and explain this in the vignette.

Regarding the Illumina annotation packages, they are used in the application. Minfi, (the package that I used for array normalization) automatically load the annotation package that it needs depending on the data uploaded. Since my package can be used to analyze 450k or EPIC arrays (with different annotation packages), loading explicitly both packages would produce an increase in the RAM usage. However, the annotation packages (such as IlluminaHumanMethylation450kanno.ilmn12.hg19) should be installed because, otherwise, the application fails when minfi try to load them.

mtmorgan commented 3 years ago

The annotation packages should be in Suggests:, relying either on minfi code or your own code to tell the user to make sure to install the packages they need for their analysis. For what it's worth, if they are in the 'Imports:' field, then they are as expensive, in terms of space, as they are in the Depends: field -- in both cases, the packages are 'loaded' in to memory. 'Depends' packages are also 'attached' to the search path, but this just makes the symbols in each package visible on the command line.

bioc-issue-bot commented 3 years ago

Received a valid push on git.bioconductor.org; starting a build for commit id: ec5de02a3366c6b090391c95aa3eb74f6194b344

bioc-issue-bot commented 3 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. This link will be active for 21 days.

Remember: if you submitted your package after July 7th, 2020, when making changes to your repository push to git@git.bioconductor.org:packages/shinyepico to trigger a new build. A quick tutorial for setting up remotes and pushing to upstream can be found here.

bioc-issue-bot commented 3 years ago

Received a valid push on git.bioconductor.org; starting a build for commit id: 9eb472953d05ea7dc66dd4a186d8423495aef65d

bioc-issue-bot commented 3 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. This link will be active for 21 days.

Remember: if you submitted your package after July 7th, 2020, when making changes to your repository push to git@git.bioconductor.org:packages/shinyepico to trigger a new build. A quick tutorial for setting up remotes and pushing to upstream can be found here.

omorante commented 3 years ago

The annotation packages should be in Suggests:, relying either on minfi code or your own code to tell the user to make sure to install the packages they need for their analysis. For what it's worth, if they are in the 'Imports:' field, then they are as expensive, in terms of space, as they are in the Depends: field -- in both cases, the packages are 'loaded' in to memory. 'Depends' packages are also 'attached' to the search path, but this just makes the symbols in each package visible on the command line.

Thank you.

I think that now I have fixed both problems. I have added a validate in the shiny to check if the proper annotation is installed.

And I have removed the set.seed and explain that in the vignette.

bioc-issue-bot commented 3 years ago

Received a valid push on git.bioconductor.org; starting a build for commit id: 2407976457666183a3ad339a1109a5c5ba6fd252

bioc-issue-bot commented 3 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. This link will be active for 21 days.

Remember: if you submitted your package after July 7th, 2020, when making changes to your repository push to git@git.bioconductor.org:packages/shinyepico to trigger a new build. A quick tutorial for setting up remotes and pushing to upstream can be found here.

bioc-issue-bot commented 3 years ago

Received a valid push on git.bioconductor.org; starting a build for commit id: 27a2de3c6a77644aaa22be238d8cec6fdd9d4e9b

mtmorgan commented 3 years ago

DESCRIPTION

good

vignettes

shiny_epico.Rmd:60 please replace or add the standard Bioconductor package installation instructions
shiny_epico.Rmd:76 I don't think there's value in reiterating the dependencies here (they will be installed anyway), or at least do so programmatically, e.g., using packageDescription("shinyepico")$Depends, etc.
good vignette for describing the app. My preference would be to have the functionality of the app fully reproducible from the command line, and to include the command line code correspondign to each step in the app reproduced (and evaluated) in the vignette.

R

I like the way the app is integrated into the R/ directory, so all code is subject to R CMD check, etc. The separation of the server / ui and the R code implementation seems very good.
utils_analysis.R:115 message(paste(...)) is often redundant with message(...), since message() calls paste0(...) internally.
utils_download.R I'm concerned that your direct parsing of BED files confuses the 0-based, half-open intervals of the bed format with the 1-based, closed intervals that are adopted by interoperable Bioconductor packages.

omorante commented 3 years ago

Thank you @mtmorgan

I have some doubts about specific issues:

Regarding the vignette, I decided to make it static because my package at the moment only exports the run_shinyepico function, and it is intended to be used interactively. Should I export all the internal functions and use them in the vignette? I could try it, but maybe the building of the vignette would be too long.

Regarding the BED parsing, I export the BED files 0-based. Where is this problem of confusing 0-based with 1-based?

Maybe, here?

ann_granges <- data.frame(
            chr = annotation$chr,
            start = annotation$pos - 1,
            end = annotation$pos,
            name = row.names(annotation)
          )

     ann_granges <- GenomicRanges::makeGRangesFromDataFrame(
            ann_granges,
            starts.in.df.are.0based = TRUE,
            keep.extra.columns = TRUE
          )

Here, I make the data.frame 0-based, and use the parameter "starts.in.df.are.0based". I could change that.

Thanks for pointing out these problems. I will start to fix them soon.

mtmorgan commented 3 years ago

My suggestion with the vignette is driven by the need to actually test, as part of the package build / check process, that your code works as expected. The ways to test your code are either to evaluate it (in the vignette) or to write unit tests. Many people face the challenge of writing vignettes that complete quickly enough; the best strategy is to identify a suitably small dataset that nonetheless provides useful biological insight. One consequence of a small dataset is that people can explore your software easily.

I did not look at your code closely, but know that rtracklayer::import.bed() / import() has been developed to import bed files so that the resulting ranges in R are 1-based, closed intervals, and that this representation is the standard in Bioconductor. So I was concerned that by not using rtracklayer you were needlessly risking representation to the user of intervals that were 0-based half-open, and that this representation would be confusing.

bioc-issue-bot commented 3 years ago

Received a valid push on git.bioconductor.org; starting a build for commit id: fc912ad623fd767453529bafad3a91af5d74f1a3

bioc-issue-bot commented 3 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, 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. This link will be active for 21 days.

Remember: if you submitted your package after July 7th, 2020, when making changes to your repository push to git@git.bioconductor.org:packages/shinyepico to trigger a new build. A quick tutorial for setting up remotes and pushing to upstream can be found here.

omorante commented 3 years ago

Hi @mtmorgan ,

I have started to fix the issues, but it seems like there is a problem with the build system (a dependence, mCSEA, is not available in some OS)

I am going to explain inline the changes:

DESCRIPTION

* good

vignettes

* shiny_epico.Rmd:60 please replace or add the standard Bioconductor package installation instructions

I have added the standard Bioconductor package installation instructions. I have kept also the GitHub installation for the moment (because the package is also under review in a journal, and at the moment the installation through Bioconductor is not possible)

* shiny_epico.Rmd:76 I don't think there's value in reiterating the dependencies here (they will be installed anyway), or at least do so programmatically, e.g., using `packageDescription("shinyepico")$Depends`, etc.

I have used packageDescription("shinyepico")$Imports, etc.

* good vignette for describing the app. My preference would be to have the functionality of the app fully reproducible from the command line and to include the command line code corresponding to each step in the app reproduced (and evaluated) in the vignette.

Thanks. I think that a vignette with all the steps of the package would be very difficult to do. For example, I check the vignette of the minfi package, and it also does not check all their options. I think that the time to calculate all is very long, and if I use a subset (for example a few CpGs), then the example will not be applicable if you use shiny through the web interface. I think that it is important to make the example useful for the shiny web interface because it is the main use of the application.

However, I am incorporating unit tests to test all the main steps of the analysis pipeline. As I have already all the steps in different functions, it is not difficult to add some tests with sample data (I am using the minfiData package). I have tested all the steps, but probably I could add more tests in each step.

R

* I like the way the app is integrated into the R/ directory, so all code is subject to R CMD check, etc. The separation of the server / ui and the R code implementation seems very good.

Thank you!

* utils_analysis.R:115 `message(paste(...))` is often redundant with `message(...)`, since `message()` calls `paste0(...)` internally.

Fixed.

* utils_download.R I'm concerned that your direct parsing of BED files confuses the 0-based, half-open intervals of the bed format with the 1-based, closed intervals that are adopted by interoperable Bioconductor packages.

I am not importing BED files, my application only exports bed files. I create a bed file from the annotation of the 450k/EPIC arrays, in a 0-based format. So, I can not use rtracklayer::import.bed().

bioc-issue-bot commented 3 years ago

Your package has been accepted. It will be added to the Bioconductor nightly builds.

Thank you for contributing to Bioconductor!

mtmorgan commented 3 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/omorante.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("shinyepico"). The package 'landing page' will be created at

https://bioconductor.org/packages/shinyepico

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

ShinyÉPICo #1764