glmGamPoi - Githubissues

const-ae commented 4 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/const-ae/glmGamPoi

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

Hi @const-ae

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: glmGamPoi
Type: Package
Title: Fit a Gamma-Poisson Generalized Linear Model
Version: 0.99.0
Authors@R: c(person("Constantin", "Ahlmann-Eltze", email = "artjom31415@googlemail.com", 
          role = c("aut", "cre"), comment = c(ORCID = "0000-0002-3762-068X")),
   person("Michael", "Love", email="michaelisaiahlove@gmail.com", role = "ctb"))
Description: Fit linear models to overdispersed count data.
     The package can estimate the overdispersion and fit repeated models
     for matrix input. It is designed to handle large input datasets as they
     typically occur in single cell RNA-seq experiments.
License: GPL-3
Encoding: UTF-8
LazyData: true
SystemRequirements: C++11
Depends: R (>= 3.6.0)
Suggests: 
    testthat (>= 2.1.0),
    zoo,
    DESeq2,
    edgeR,
    beachmat,
    MASS,
    statmod,
    ggplot2,
    bench,
    BiocParallel,
    knitr,
    rmarkdown,
    BiocStyle
LinkingTo: 
    Rcpp,
    RcppArmadillo,
    beachmat (>= 2.0.0)
Imports: 
    Rcpp,
    pracma,
    DelayedMatrixStats,
    DelayedArray,
    HDF5Array,
    SummarizedExperiment,
    methods,
    stats,
    utils
Roxygen: list(markdown = TRUE)
RoxygenNote: 7.0.2
URL: https://github.com/const-ae/glmGamPoi
BugReports: https://github.com/const-ae/glmGamPoi/issues
biocViews: Regression, RNASeq, Software, SingleCell
VignetteBuilder: knitr

bioc-issue-bot commented 4 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 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: "ERROR, 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:

21f39e4 Fix some problems in documentation 49bc627 Bump version

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: "ERROR, 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:

64a557f Fix more problems in documentation 6409e88 Bump version

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:

e882432 Add .github to .Rbuildignore e64c4c7 Bump version

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.

lshep commented 4 years ago

Thanks for your submission. Please see initial review comments:

DESCIRPTION

[ ] Remove lazyData: true we have rarely found this useful and in our experience can slow installation

NAMESPACE

[ ] Is there a reason you choose S3 implementation rather than S4? We generally prefer S4 implementation and would highly suggest transitioning the code from S3 to S4.

NEWS

[ ] Please see suggested formatting. We don't recommend having the package name as the title as it is duplicated information ?news we recommend changes in version.

inst

[ ] What are these and where are they used?

vignette

[ ] Change the name. We highly recommend the name of the package as it is intuitive for a user to do vigentte("glmGamPoi") . Regardless it should be less generic. Something like Quickstart could be a vignette in multiple packages and depending on how they loaded the packages into their R session cause your vignette to not actually display.
[ ] Your vignette is minimal while your README is more extensive. You can keep this quickstart basic vignette but you will need a more comprehensive vignette as well.
[ ] Include Bioconductor installation instructions in all vignettes
[ ] In your vignette you do c(fit) yet c() is a standard concatenation function. Change the name of this function to accurate display what it is doing.
[ ] You should show the real dataset in the vignette as well as the README. README code is not run or tested and using simulated data doesn't always catch errors in the code like the code found in examples/vignette/tests. How long does it take your function to run? Is there not a smaller real dataset that can be used? The README code while it took a bit of time did not seem like it would be unreasonable to run in the vignette and still be under the expected time constraint? You could always load the full dataset and truncate it to a smaller set like you did for the benchmarking in the README

README

[ ] Include Bicoonductor installation instructions too

R code / man pages

General Notes

[ ] As noted above we would highly recommend using S4 instead of S3. You will need to make a strong case for not transitioning to S4 classes.

glmGamPoi-package

[ ] (Optional) might be nice to have the glmGamPoi.Rd man page that gives a summary of the package and list the main functions.

glm_gp

[ ] Do any of the other arguments need parameter checking to make sure the user enters valid input?

print_model

[ ] have one paste0 call instead of multiple in format.glmGamPoi

When ready please remember to do a version bump to kick off a new build and comment back here that you are ready for a re-review with a summary of changes.

Cheers!

bioc-issue-bot commented 4 years ago

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

5350212 Remove LazyData from DESCRIPTION 14f5669 Fix NEWS file 50fded8 Rename vignette to less generic name b58ed31 Use same code in README and vignette 00faec4 Bump version

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.

bioc-issue-bot commented 4 years ago

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

7d98cec Bump version again

bioc-issue-bot commented 4 years ago

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

07ccc73 Fix bug in glm_gp_impl where do_cox_reid_adjustmen... fd1a38e Simplify format.glmGamPoi 09cee0e Bump version

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.

const-ae commented 4 years ago

Hi Lori,

thank you very much for your review. I hope my comments clarify some of the decision I took. But feel free to correct me if I misunderstood something :)

Is there a reason you choose S3 implementation rather than S4? We generally prefer S4 implementation and would highly suggest transitioning the code from S3 to S4.

I wanted to keep the overhead (in terms of cognitive load and additional getter methods) for the package as low as possible. And in the spirit of "Everything should be made as simple as possible, but not simpler", I felt that S3 achieves exactly that.

inst What are these and where are they used?

The inst folder contains the C++ header files that I want to make available for other packages, so they don't have to repeat the same implementation. For example the fisher_scoring_qr_step() function can be useful in and of itself. I followed the example of Aaron Lun's beachmat.

In your vignette you do c(fit) yet c() is a standard concatenation function

I use c() for its secondary function of stripping an object of all attributes, excepts names.

Do any of the other arguments need parameter checking to make sure the user enters valid input?

As far as I can tell, no. Note that a lot of parameters are validated in glm_gp_impl().

I fixed the following issues:

Remove lazyData
NEWS
vignette. I use the same code now in the README and the glmGamPoi-vignette. Thus, the vignette is now more extensive and uses real world data. I also included the Bioconductor installation instructions
one paste0() in print_model

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.

mtmorgan commented 4 years ago

For the c() argument, why not just use attributes(x) = NULL which is explicit in what it is doing?

lshep commented 4 years ago

Please see @mtmorgan comment above. I still would insist on renaming the exported function c() to something else or enhancing the functionality to be able to concat two of your objects. Basic R programming teaches that function as a listing and concatenation function, not strictly for formatting.

const-ae commented 4 years ago

Hi Lori, thank you so much for your comments, the last week was quite busy, so I didn't manage to take care of the issues, but I will look into the comments and adapt the code in the next two days.

bioc-issue-bot commented 4 years ago

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

83dc724 Add as.list.glmGamPoi() function d6031c7 Use as.list(fit) instead of c(fit) to strip cl... 29b77c3 Bump version

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: "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 4 years ago

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

01d2aa6 Fix documentation for as.list.glmGamPoi

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: "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 4 years ago

The NEWS file ERROR is on our end. We have a fix implemented in linux and there will be a fix on windows by later this afternoon. I will kick off a manual build so there is at least an accurate linux build. it should post shortly.

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.

lshep commented 4 years ago

NEWS file ERROR on our end is fixed. Let me know when you are ready for a re-review

const-ae commented 4 years ago

Thank you for the quick fix.

Compared with the original review, I have now fixed the following issues:

Removed LazyDate from DESCRIPTION
Fixed NEWS file
Renamed vignette to glmGamPoi.Rmd
Extended the content of the vignette which is now identical to the README
Use the BioConductor installation instructions
Removed the confusing c(fit) pattern with as.list(fit) which expresses much better what I want to do
Use the TENxPBMCData in the vignette
Fixed the repeated paste0() calls in print_model.R

About the two question you raised:

inst What are these and where are they used?

The inst folder contains the C++ header files that I want to make available for other packages, so they don't have to repeat the same implementation. For example the fisher_scoring_qr_step() function can be useful in and of itself. I followed the example of Aaron Lun's beachmat.

Do any of the other arguments need parameter checking to make sure the user enters valid input?

As far as I can tell, no. Note that a lot of parameters are validated in glm_gp_impl().

lshep commented 4 years ago

Thanks. changes look good. thank you.

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

https://bioconductor.org/packages/glmGamPoi

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

glmGamPoi #1431