Bioconductor / Contributions

Contribute Packages to Bioconductor
134 stars 33 forks source link

UCell #2520

Closed mass-a closed 2 years ago

mass-a commented 2 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 questions/help about the submission process, including questions about the output of the automatic reports generated by the SPB (Single Package Builder), please use the #package-submission channel of our Community Slack. Follow the link on the home page of the Bioconductor website to sign up.

bioc-issue-bot commented 2 years ago

Dear @mass-a,

The package version number, '1.99.0', does not start with 0. Expecting format: '0.99.z' for new packages. Starting with non-zero x of 'x.y.z' format is generally only allowed if the package has been pre-released.

We recommend fixing the version number. See Bioconductor version numbers Please also consider running BiocCheck::BiocCheck('new-package'=TRUE) on your package to look for other Bioconductor package requirements.

bioc-issue-bot commented 2 years ago

Hi @mass-a

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: UCell
Type: Package
Title: Rank-based signature enrichment analysis for single-cell data
Version: 1.99.0
Authors@R: c(
  person('Massimo', 'Andreatta', 
     email = 'massimo.andreatta@unil.ch',
     role = c('aut','cre'),
     comment = c(ORCID = '0000-0002-8036-2647')),
  person('Santiago', 'Carmona', 
     email = 'santiago.carmona@unil.ch',
     role = c('aut'),
     comment = c(ORCID = '0000-0002-2495-0671'))
  )
Description: UCell is a package for evaluating gene signatures in single-cell datasets. 
    UCell signature scores, based on the Mann-Whitney U statistic, are robust to dataset size and heterogeneity, and their calculation
    demands less computing time and memory than other available methods, enabling the processing of large datasets in a few minutes even
    on machines with limited computing power. UCell can be applied to any single-cell data matrix, and includes functions to directly
    interact with SingleCellExperiment and Seurat objects.
URL: https://github.com/carmonalab/UCell
Depends: R(>= 4.1.0)
Imports: methods,
    data.table(>= 1.13.6),
    Matrix,
    BiocParallel,
    SingleCellExperiment,
    SummarizedExperiment
Suggests: Seurat,
    scater,
    scRNAseq,
    reshape2,
    patchwork,
    ggplot2,
    BiocStyle,
    knitr,
    rmarkdown
biocViews: SingleCell,
    GeneSetEnrichment,
    Transcriptomics,
    GeneExpression,
    CellBasedAssays
VignetteBuilder: knitr
License: GPL-3 + file LICENSE
Encoding: UTF-8
LazyData: FALSE
Roxygen: list(markdown = TRUE)
RoxygenNote: 7.1.2
mass-a commented 2 years ago

I pushed an update to the origin repo aimed at improving interoperability (v1.99.1).

UCell signature scoring can now take directly as input a SingleCellExperiment object, and returns results in a SingleCellExperiment structure as well. This functionality was previously available, but unnecessarily stored in a separate function.

Cheers!

mass-a commented 2 years ago

Hello @vjcitn what else should be done from our side to get the package into the review stage?

Thanks, -m

bioc-issue-bot commented 2 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 2 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. 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/UCell to trigger a new build. A quick tutorial for setting up remotes and pushing to upstream can be found here.

bioc-issue-bot commented 2 years ago

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

bioc-issue-bot commented 2 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/UCell to trigger a new build. A quick tutorial for setting up remotes and pushing to upstream can be found here.

bioc-issue-bot commented 2 years ago

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

bioc-issue-bot commented 2 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/UCell to trigger a new build. A quick tutorial for setting up remotes and pushing to upstream can be found here.

mass-a commented 2 years ago

Thanks in advance for taking the time to review our package!

There is now 1 warning related to the package version. The method has been published and cited by other publications with versions > 1.0, it would probably be confusing to roll back the version numbering. Is that an option to come into BioC starting with version 2.0?

-m

lazappi commented 2 years ago

This is probably a question for the core team. Maybe @lshep can jump in?

lshep commented 2 years ago

Yes we can make an exception for this. The recommended action is what you currently have: 1.99.0 so that when officially released in Bioconductor it will automatically be bumped to 2.0.0.

lazappi commented 2 years ago

Hi @mass-a

Thanks for submitting UCell :tada:! Below is my review of your package. Please reply here if anything is unclear or needs any further explanation.

What next?

Please address the comments as best as you can. When you are ready for me to check the package again please reply to let me know with a summary of changes you have made or any other responses.

Luke

Review

Key: :rotating_light: Required :warning: Recommended :green_circle: Optional :question: Question

General package development

DESCRIPTION file

NAMESPACE file

The NEWS file

Package data

Documentation

README

Vignette

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

Man pages

Code

R

bioc-issue-bot commented 2 years ago

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

bioc-issue-bot commented 2 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/UCell to trigger a new build. A quick tutorial for setting up remotes and pushing to upstream can be found here.

mass-a commented 2 years ago

Hello Luke, thanks for the review!

Please find my responses below.

General package development

* [ ]  ⚠️ There are some notes from `BiocCheck::BiocCheck()` in the build report. Please address these as much as possible.

Most notes should be solved now. There are a few long lines and lines not indented by 4, but these are mostly auto-generated by roxygen for the man pages.

DESCRIPTION file

* [ ]  ⚠️ It is recommended to add a URL to the `BugReports` field. Usually this is for the package GitHub issues page or the Bioconductor support form.

Done.

* [ ]  ⚠️ It is recommended to add a URL to the `URL` field. Usually this is to the package GitHub repository (you can also give multiple URLs).

Done.

NAMESPACE file

* [ ]  ⚠️ Function names should use `camelCase` or `snake_case` rather than a mix or `UpperCamelCase`. For example `storeUCellRankings()` rather than `StoreRankings_UCell()`.

This is the only point I am reluctant about. Renaming all the functions would probably cause problems to active users and compatibility with existing pipelines. There are at least two other packages that I know of that depend on UCell (escape and scGate). If there is flexibility on this point I would prefer not to rename the functions.

The NEWS file

* [ ]  🟢 Please try to keep the `NEWS` file up to date

Added a new entry with the most recent updates.

Package data

* [ ]  🚨 Please add more documentation for the sample dataset. This should include information about the source of the data, any processing done to it etc.

Added a short description of the test dataset and reference to original publication.

Documentation

README

* [ ]  🚨 Please use Bioconductor installation instructions in the `README`

Added Bioc installation instructions.

Vignette

* [ ]  🚨 Please add a namespace check for **{BiocManager}** to the Bioconductor installation instructions
if (!requireNamespace("BiocManager", quietly=TRUE))
    install.packages("BiocManager")
BiocManager::install("UCell")

Done.

* [ ]  ⚠️ It is good to show how to set the parameter for running on multiple cores but 4 four cores may not always be available depending on where the vignette is being run.

I have set to eval=FALSE the chunk that shows how to run on multiple cores, so it outlines the syntax without actually running the code. I hope this is what you were suggesting.

Man pages

* [ ]  ⚠️ It is recommended to add a package man page, see https://r-pkgs.org/man.html#man-packages or `usethis::use_package_doc()`

Thanks for the suggestion, done.

* [ ]  🚨 Please make the examples in man pages runnable unless there is a reason this cannot be done

Done.

* [ ]  🚨 There are some references to the **{future}** package which should be removed as it is no longer used

Done.

Code

R

* [ ]  🚨 Please remove the `seed` argument which is no longer used

Done.

* [ ]  🚨 Please use accessor functions for Seurat objects rather than `@` or `slot()`

I have re-written the code and examples to avoid using @ and slot()

* [ ]  ⚠️ Please consider adding validity tests for function arguments

Added some parameter format checks.

* [ ]  🚨 When using **{BiocParallel}** users should be able to choose how parallelisation is done. This should be done by having a `BPPARAM` argument which takes a `BiocParallel::bpparam()` object (rather than creating it inside a function).

Good point. I have implemented both: a user can either give a BiocParallel::bpparam() object, or specify the number of cores as an integer. The vignette was updated to use the BPPARAM parameter for parallel processing.

That's it! -massimo

lazappi commented 2 years ago

Thanks! I have a few follow up comments but overall I think this is looking pretty good and we can probably approve this once you respond to these.

This is the only point I am reluctant about. Renaming all the functions would probably cause problems to active users and compatibility with existing pipelines. There are at least two other packages that I know of that depend on UCell (escape and scGate). If there is flexibility on this point I would prefer not to rename the functions.

I think this is probably reasonable as this is a more mature package than average. @lshep please jump in if the core feels more strongly about this.

Added a short description of the test dataset and reference to original publication.

Could you please add what processing has been done to the data, if any (i.e. what the matrix values are)? Also it took me a while to find where in the code this was so maybe worth moving to a datasets.R file (up to you though).

I have set to eval=FALSE the chunk that shows how to run on multiple cores, so it outlines the syntax without actually running the code. I hope this is what you were suggesting.

That's a reasonable approach I think, maybe worth mentioning in the text that it isn't run though. The other alternative I was thinking of was to run the code but just set the number of workers to 1. Slightly less good as an example but then you don't have to worry about checking the code is still up to date.

Good point. I have implemented both: a user can either give a BiocParallel::bpparam() object, or specify the number of cores as an integer. The vignette was updated to use the BPPARAM parameter for parallel processing.

Great! Maybe add to the documentation that BPPARAM overrides ncores and that if ncores is used then it uses BiocParallel::MulticoreParam().

bioc-issue-bot commented 2 years ago

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

bioc-issue-bot commented 2 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/UCell to trigger a new build. A quick tutorial for setting up remotes and pushing to upstream can be found here.

mass-a commented 2 years ago

Thanks @lazappi!

your comments are all well taken.

Could you please add what processing has been done to the data, if any (i.e. what the matrix values are)? Also it took me a while to find where in the code this was so maybe worth moving to a datasets.R file (up to you though).

Moved the test data documentation to a dedicated datasets.R file, and included details about the processing.

That's a reasonable approach I think, maybe worth mentioning in the text that it isn't run though. The other alternative I was thinking of was to run the code but just set the number of workers to 1. Slightly less good as an example but then you don't have to worry about checking the code is still up to date.

I finally went for your second suggestion: setting workers=1 and explaining in writing how the behavior can be altered to parallelise execution.

Great! Maybe add to the documentation that BPPARAM overrides ncores and that if ncores is used then it uses BiocParallel::MulticoreParam().

Updated the documentation to clarify this point, along the lines of your suggestion.

Thanks again and let me know if you see anything else.

Best, -m

lazappi commented 2 years ago

I'm happy with this so I will mark it as approved. Congratulations on getting {UCell} into Bioconductor 🎉! It can take a few days for the build system to pick things up but the package should be part of Bioconductor devel soon.

bioc-issue-bot commented 2 years ago

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

Thank you for contributing to Bioconductor!

Reviewers for Bioconductor packages are volunteers from the Bioconductor community. If you are interested in becoming a Bioconductor package reviewer, please see Reviewers Expectations.

lshep commented 2 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/mass-a.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("UCell"). The package 'landing page' will be created at

https://bioconductor.org/packages/UCell

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.