ReactomeGSA - Githubissues

jgriss commented 5 years ago

Repository: https://github.com/reactome/ReactomeGSA

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][2] instructions. My package is consistent with the Bioconductor [Package Guidelines][1].
[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][3] for issues that users may have, subscribing to the [bioc-devel][4] 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][5] (optionally [via GitHub][6]).

bioc-issue-bot commented 5 years ago

Hi @jgriss

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: ReactomeGSA
Type: Package
Title: Client for the REACTOME Analysis Service for comparative multi-omics gene set analysis
Version: 0.99.0
Authors@R: person("Johannes", "Griss", email = "johannes.griss@meduniwien.ac.at", role = c("aut", "cre"), comment = c(ORCID = "0000-0003-2206-9511"))
Description: The ReactomeGSA packages uses REACTOME's online analysis service to perform a multi-omics
    gene set analysis. The main advantage of this package is, that the retrieved results can be 
    visualized using REACTOME's powerful webapplication.
    Since REACTOME's analysis service also uses R to perfrom the actual gene set
    analysis you will get similar results when using the same packages (such as limma and edgeR) locally.
    Therefore, if you only require a gene set analysis, different packages are more suited.
License: MIT + file LICENSE
Encoding: UTF-8
LazyData: false
Imports: jsonlite, httr, progress, ggplot2, methods
RoxygenNote: 6.1.1
Suggests: 
    testthat,
    knitr,
    rmarkdown,
    ReactomeGSA.data,
    Biobase
VignetteBuilder: knitr
biocViews: GeneSetEnrichment, Proteomics, Transcriptomics, SystemsBiology, GeneExpression, Reactome
BugReports: https://github.com/reactome/ReactomeGSA/issues
URL: https://github.com/reactome/ReactomeGSA

Add SSH keys to your GitHub account. SSH keys will are used to control access to accepted Bioconductor packages. See these instructions to add SSH keys to your GitHub account.

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

jgriss commented 5 years ago

AdditionalPackage: https://github.com/reactome/ReactomeGSA.data

Data package required to run the examples and build the vignette.

bioc-issue-bot commented 5 years ago

Hi @jgriss,

Starting build on additional package https://github.com/reactome/ReactomeGSA.data.

IMPORTANT: Please read the instructions for setting up a push hook on your repository, or further changes to your additional package repository will NOT trigger a new build.

The DESCRIPTION file of this additional package is:

Package: ReactomeGSA.data
Type: Package
Title: Companion data package for the ReactomeGSA package
Version: 0.1.0
Authors@R: person("Johannes", "Griss", email = "johannes.griss@meduniwien.ac.at", role = c("aut", "cre"), comment = c(ORCID = "0000-0003-2206-9511"))
Description: Companion data sets to showcase the functionality of the ReactomeGSA package.
This package contains proteomics and RNA-seq data of the melanoma B-cell induction study by Griss et al.
License: Artistic-2.0
Encoding: UTF-8
Depends: R (>= 3.6), limma, edgeR
RoxygenNote: 6.1.1
biocViews: ExpressionData, RNASeqData, Proteome, Homo_sapiens_Data
BugReports: https://github.com/reactome/ReactomeGSA.data
URL: https://github.com/reactome/ReactomeGSA.data/issues

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:

3beaba8 Moved to version 0.99.1

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:

933ba20 Fixed documentation inconsistencies

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:

d8e4a2f Moved to 0.99.3

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

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

fc922e6 Moved to version 0.99.0 6fe8d91 Merge remote-tracking branch 'refs/remotes/origin/...

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:

49b67e3 Addded ReactomeGSA as dependency.

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:

49389f8 Fixed reference to ReactomeAnalysisResult class

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

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

526cd9c Fixed bugs in plotting functions

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

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

e0a7138 Fixes progress bar update bug 532acc5 Fixes bug that dataset level parameters were not c... e06b6b8 Moved to version 0.99.5 Fixed bug that pathways d...

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:

50a7a62 Minor bugfixes

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.

hpages commented 5 years ago

Hi @jgriss,

Thanks for submitting the ReactomeGSA package.

Here are a few things that would require your attention before we can consider adding ReactomeGSA (and ReactomeGSA.data) to Bioconductor:

Installing the package produces the following NOTEs which should not be ignored:

hpages@spectre:~$ bdevinst ReactomeGSA_0.99.6.tar.gz 
* installing to library ‘/home/hpages/R/R-3.6.r76454/library’
* installing *source* package ‘ReactomeGSA’ ...
** using staged installation
** R
** byte-compile and prepare package for lazy loading
NOTE: arguments in definition for validity method for class 'ReactomeAnalysisRequest' changed from (request) to (object)
in method for ‘add_dataset’ with signature 
‘request="ReactomeAnalysisRequest",expression_values="EList"’: no definition for class “EList”
in method for ‘add_dataset’ with signature 
‘request="ReactomeAnalysisRequest",expression_values="DGEList"’: no definition for class “DGEList”
in method for ‘add_dataset’ with signature 
‘request="ReactomeAnalysisRequest",expression_values="ExpressionSet"’: no definition for class “ExpressionSet”
** help
*** installing help indices
** building package indices
** installing vignettes
** testing if installed package can be loaded from temporary location
** testing if installed package can be loaded from final location
** testing if installed package keeps a record of temporary installation path
* DONE (ReactomeGSA)

Please remove the package startup message. These banners are strongly discouraged in Bioconductor. If you really really want to have one, please make it a 1-liner or 2-liner only.
You sometimes spell the package ReactomeGSA and sometimes REACTOMEgsa. Please choose one spelling and stick to it.
Please update the Installation section to reflect what the user will need to do once the package is in Bioconductor. This will be a 3-liner only:
```
if (!requireNamespace("BiocManager", quietly = TRUE))
    install.packages("BiocManager")

BiocManager::install("ReactomeGSA")
```
Adding a link to https://bioconductor.org/install/ is also a good idea.
It is generally considered distracting and not useful to explictely set optional arguments to their default value like in get_reactome_methods(print_methods = TRUE, return_result = FALSE).
The output produced by certain commands takes too much space in the vignette. For example the output of get_reactome_methods(print_methods = TRUE, return_result = FALSE) fills more than 5 screens on my laptop. This is too much and not useful in the context of a vignette.

Bioconductor strongly discourages the direct use of new() by end users to create new S4 objects. Instead a constructor function named as the class should be provided:

## End users should be able to do this:
my_request <- ReactomeAnalysisRequest(method = "Camera")
## instead of this:
my_request <- new("ReactomeAnalysisRequest", method = "Camera")

In addition the constructor function should fail graciously with an informative error message when the supplied input is invalid:

## This should fail with an informative error message instead of the
## obscure warning below:
my_request <- new("ReactomeAnalysisRequest", method = letters)
# Warning message:
# In if (nchar(request@method) < 1) { :
#   the condition has length > 1 and only the first element will be used

I don't think this is actually keeping genes with >= 100 transcripts in total:

# only keep genes with >= 100 transcripts in total
total_transcripts <- rowSums(griss_melanoma_rnaseq$counts)
griss_melanoma_rnaseq <- griss_melanoma_rnaseq[total_transcripts >= 100, ]

You probably meant reads instead of transcripts.

Thanks, H.

jgriss commented 5 years ago

Hi @hpages,

Thanks a lot for your feedback! That's very helpful! I'll take care of these issues as quickly as possible.

I only have one question about Point 1: As you saw, I tried to support the most common data structures for transcriptomics and microarray datasets. Do you know how I can get these notes removed? I did not want to add all of these packages as dependencies.

Thanks a lot!

Johannes

bioc-issue-bot commented 5 years ago

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

530cf23 Adapted output of get_reactome_methods and moved t... e977fc5 Addressed Bioconductor review

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.

jgriss commented 5 years ago

Hi @hpages,

I believe that I was able to address all your comments.

In one method (add_dataset) I left the explicit default values for two parameters to highlight that they normally do not have to be set. In this case, I believe that it adds clarity. Otherwise, I fully agree and removed all of them.

Apart from that I implemented all your suggestions.

The two Notes that you mentioned in point 1 are no longer shown in the output of the build system. My system also did not display them when I checked the package. I hope, that this issue is therefore addressed as well.

Kind regards, Johannes

hpages commented 5 years ago

Hi Johannes,

Thanks for the changes. We're almost there:

Item 1: This item still needs to be addressed.
Item 5: My point was that in your function calls you don't need to explicitly set the arguments to their default values. But instead of that you removed the argument defaults from the function definition. Unfortunately this makes the software less user-friendly. Please put the argument defaults back in the function definition. Then please simplify your function calls by not explicitly setting arguments to their default values.

Some new issues:

The following files in your vignettes/ folder should not be added to your git repository: using-reactomegsa.R and using-reactomegsa.html. Please remove them.
You have duplicate lines in the Usage and Value sections of the man page for ?ReactomeAnalysisRequest. Also in the Examples section, the same code is repeated twice.

Thanks, H.

bioc-issue-bot commented 4 years ago

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

65b33ca Removed unnecessary files from vignettes/ f9cb774 Fixed duplicate lines in ReactomeAnalysisRequest d... 48eb1ca Moved default values to function definitions again fabd745 Moved to version 0.99.9 addressing Bioconductor re...

bioc-issue-bot commented 4 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 4 years ago

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

0e1eb88 Added missing documentation

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

jgriss commented 4 years ago

Hi @hpages,

Thanks a lot for your feedback!

I tried to address all your raised points. There is one thing that still remains with point 1 though:

in method for ‘add_dataset’ with signature ‘request="ReactomeAnalysisRequest",expression_values="EList"’: no definition for class “EList”
in method for ‘add_dataset’ with signature ‘request="ReactomeAnalysisRequest",expression_values="DGEList"’: no definition for class “DGEList”
in method for ‘add_dataset’ with signature ‘request="ReactomeAnalysisRequest",expression_values="ExpressionSet"’: no definition for class “ExpressionSet”

This is caused by the fact that I do not import the respective classes from limma, edgeR, and BioBase. If I add the import statement, the message is gone but then I have to add these packages as requirements - which I do not want to since these functions are only sensible if the respective packages are present. Therefore, I feel that these message can (and in this case should) be ignored.

In case you don't agree, please let me know how you'd solve this.

Thanks a lot!

Johannes

hpages commented 4 years ago

Hi Johannes,

Thanks for the changes.

I understand for the NOTEs. I guess there's not much you can do about them.

I'll check you latest changes later today.

H.

hpages commented 4 years ago

Everything looks good. Thx for your contribution.

H.

bioc-issue-bot commented 4 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 4 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/jgriss.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("ReactomeGSA"). The package 'landing page' will be created at

https://bioconductor.org/packages/ReactomeGSA

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 commented 4 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/jgriss.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("ReactomeGSA.data"). The package 'landing page' will be created at

https://bioconductor.org/packages/ReactomeGSA.data

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

ReactomeGSA #1170