Bioconductor / Contributions

Contribute Packages to Bioconductor
134 stars 33 forks source link

SPOTlight #2488

Closed MarcElosua closed 2 years ago

MarcElosua 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

Hi @MarcElosua

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: SPOTlight
Version: 0.99.0
Type: Package
Title: `SPOTlight`: Spatial Transcriptomics Deconvolution
Description: `SPOTlight`provides a method to deconvolute spatial transcriptomics
    spots using a seeded NMF approach along with visualization tools to assess
    the results. Spatially resolved gene expression profiles are key to
    understand tissue organization and function. However, novel spatial 
    transcriptomics (ST) profiling techniques lack single-cell resolution and 
    require a combination with single-cell RNA sequencing (scRNA-seq) 
    information to deconvolute the spatially indexed datasets. Leveraging the 
    strengths of both data types, we developed SPOTlight, a computational tool 
    that enables the integration of ST with scRNA-seq data to infer the location
    of cell types and states within a complex tissue. SPOTlight is centered 
    around a seeded non-negative matrix factorization (NMF) regression, 
    initialized using cell-type marker genes and non-negative least squares
    (NNLS) to subsequently deconvolute ST capture locations (spots).
Authors@R: c(
  person("Marc", "Elosua-Bayes", email="elosua.marc@gmail.com", role=c("aut", "cre")),
  person("Helena L.", "Crowell", email="helena@crowell.eu", role="aut"))
Depends: R (>= 4.1)
Imports:
  ggplot2,
  NMF,
  Matrix,
  matrixStats,
  nnls,
  SeuratObject,
  SingleCellExperiment,
  stats
Suggests:
  BiocStyle,
  ExperimentHub,
  DelayedArray,
  ggcorrplot,
  grid,
  igraph,
  jpeg,
  knitr,
  methods,
  png,
  scater,
  scatterpie,
  scran,
  Seurat,
  SeuratData,
  SpatialExperiment,
  SummarizedExperiment,
  TabulaMurisSenisData,
  TENxVisiumData,
  testthat
biocViews: 
  SingleCell, 
  Spatial,
  StatisticalMethod
License: GPL-3
Encoding: UTF-8
RoxygenNote: 7.1.2
VignetteBuilder: knitr
URL: https://github.com/MarcElosua/SPOTlight
BugReports: https://github.com/MarcElosua/SPOTlight/issues
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/SPOTlight 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: 43d036e4d36448e8f194a37960911afa8a05c3b3

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.

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

LiNk-NY commented 2 years ago

Hi Marc, @MarcElosua

Thank you for your submission. Please see the review below.

Best regards, Marcel


SPOTlight #2488

DESCRIPTION

NAMESPACE

vignettes

Minor:

R

code coverage

> covr::package_coverage()
SPOTlight Coverage: 91.19%
R/plotSpatialScatterpie.R: 53.33%
R/plotImage.R: 82.50%
R/SPOTlight.R: 90.32%
R/utils.R: 95.76%
R/data.R: 98.41%
R/AllGenerics.R: 100.00%
R/plotCorrelationMatrix.R: 100.00%
R/plotInteractions.R: 100.00%
R/plotTopicProfiles.R: 100.00%
bioc-issue-bot commented 2 years ago

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

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, 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. 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/SPOTlight 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: eab8b0bc7014e886d9d62fa5ef6a3beae00bbe10

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

HelenaLC commented 2 years ago

Dear Marcel, would you be able to advise us regarding:

* Checking DESCRIPTION/NAMESPACE consistency...
    * WARNING: Import NMF, Matrix, matrixStats, SeuratObject, stats in
      NAMESPACE as well as DESCRIPTION.
    * WARNING: Import colSums, rowSums, basis, coef, nmf, nmfModel,
      GetAssayData, GetImage, GetTissueCoordinates, Idents, Images,
      cbind, colLabels, counts, colMedians, rowAlls, rowSds, aggregate,
      rnbinom, runif in DESCRIPTION as well as NAMESPACE.

I can see all mentioned packages/functions in the NAMESPACE as well as DESCRIPTION, and we are not sure how recent changes could have caused these warnings. Any hints would be greatly appreciated!

As a side note, there's also this at the top of the BiocCheck part of the build report, but we don't know what that means:

* Checking Package Dependencies...
Warning in system2(cmd, args, stdout = TRUE, stderr = FALSE, env = "R_DEFAULT_PACKAGES=NULL") :
  running command 'R_DEFAULT_PACKAGES=NULL '/home/biocbuild/bbs-3.15-bioc/R/bin/R' -q --vanilla --slave -f /home/biocbuild/bbs-3.15-bioc/R/library/BiocCheck/script/checkBadDeps.R --args "/home/pkgbuild/packagebuilder/workers/jobs/2488/eab8b0bc7014e886d9d62fa5ef6a3beae00bbe10/SPOTlight_0.99.3.tar.gz" "/tmp/RtmpKtgCKP/file122d5e2795c7d2/lib" 2>/dev/null' had status 1
LiNk-NY commented 2 years ago

Hi Helena, @HelenaLC

Thanks for pointing this out. That was a bug in BiocCheck. It has been fixed in version 1.31.31.

As a reminder, please see https://contributions.bioconductor.org/r-code.html?q=methods#methods-development WRT creating methods for external classes.

I will have to take a closer look at the warning on the BBS. Please ignore for now.

bioc-issue-bot commented 2 years ago

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

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, 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/SPOTlight 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

This issue is being closed because there has been no progress for an extended period of time. You may reopen the issue when you have the time to actively participate in the review / submission process. Please also keep in mind that a package accepted to Bioconductor requires a commitment on your part to ongoing maintenance.

Thank you for your interest in Bioconductor.

MarcElosua commented 2 years ago

Apologies for the delay in answering to the comments, finishing up a round of modifications and will update by the end of the week.

MarcElosua commented 2 years ago

I want to update

LiNk-NY commented 2 years ago

Hi @MarcElosua, You should be able to re-open the issue once and push changes once you are ready. Best, Marcel

bioc-issue-bot commented 2 years ago

Dear @MarcElosua ,

We have reopened the issue to continue the review process. Please remember to push a version bump to git.bioconductor.org to trigger a new build.

bioc-issue-bot commented 2 years ago

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

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, skipped". 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/SPOTlight to trigger a new build. A quick tutorial for setting up remotes and pushing to upstream can be found here.

MarcElosua commented 2 years ago

Here are some comments on previous your previous round of revisions @LiNk-NY

NAMESPACE

Since these functions are used to generate mock data for some function's examples we decided to export them. Since they are not meant to be used in a real life setting we have made it clear in the function's documentation.

vignettes

Done

In this case, since the marker genes can come in varying data structures - list, dataframe... we want to give the user the flexibility to choose the source of the marker genes. Therefore, we propose this in the vignette as an example of how the mgs should look when inputing it to SPOTlight.

Done

Minor:

Done

Done

R

Done

We fixed the issue making the grep that checks for the file extension case insensitive. We used an S4 method for this function since the input can be a character path, a Spatial Experiment object, a Seurat object, an array or a rastergrob. Could you confirm this is an appropriate use of S4 methods here?

We understand the reasoning behind this comment. In this package we aim to be flexible to various types of common input such as SCE, SP and Seurat. However we want to limit the dependencies of SPOTlight. Being SCE, SP and Seurat packages with a large amount of dependencies what would you suggest to do here? For example SCE and SE users might no be interested in having Seurat and viceversa. If this is required would you suggest moving Seurat, SCE and SPE from suggests to dependencies?

Could you clarify if you were suggesting to get rid of .test_install. We now use requireNamespace to test if a package is installed. We decided to keep the .test_installed to give an informative error message to the user.

Done

Done, changed in the function plotTopicProfiles.

Additions

We have added 2 new functions

bioc-issue-bot commented 2 years ago

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

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.

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

LiNk-NY commented 2 years ago

Hi Marc, @MarcElosua

  • Please make these functions more robust and consider creating plain functions rather than S4 methods on generic S3 classes like character. A file with different case breaks plotImage:

We fixed the issue making the grep that checks for the file extension case insensitive. We used an S4 method for this function since the input can be a character path, a Spatial Experiment object, a Seurat object, an array or a rastergrob. Could you confirm this is an appropriate use of S4 methods here?

We understand the reasoning behind this comment. In this package we aim to be flexible to various types of common input such as SCE, SP and Seurat. However we want to limit the dependencies of SPOTlight. Being SCE, SP and Seurat packages with a large amount of dependencies what would you suggest to do here? For example SCE and SE users might no be interested in having Seurat and viceversa. If this is required would you suggest moving Seurat, SCE and SPE from suggests to dependencies?

These are all related. The general recommendation is to avoid creating methods for classes that are not your own. If for example, SpatialExperiment decides to export a plotImage method, then that would create a conflict with your method. Also shouldn't this method live in the SpatialExperiment's namespace rather than in another package (yours)?

The alternative is to create a plain plotImage function that has a helper for each class of input object. Dependencies won't be a problem if you check for imports as you have with .test_installed.

MarcElosua commented 2 years ago

Hi @LiNk-NY ,

Thanks a lot for your feedback. We have applied the changes you recommended to plotImage. Before we re-submit, should we follow the same approach for trainNMF, runDeconvolution, and SPOTlight? These three function names are unlikely to be used by either Seurat or SpatialExperiment and they are central to our package.

Thanks!

bioc-issue-bot commented 2 years ago

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

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.

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

LiNk-NY commented 2 years ago

Hi Marc, @MarcElosua

Thanks a lot for your feedback. We have applied the changes you recommended to plotImage. Before we re-submit, should we follow the same approach for trainNMF, runDeconvolution, and SPOTlight? These three function names are unlikely to be used by either Seurat or SpatialExperiment and they are central to our package.

Ideally, yes. That's the recommended practice. And for some classes you don't need to do much other than run as.matrix on them which in turn makes your code more flexible to new classes. If the operation fails, you can provide a helpful error message as well.

-Marcel

MarcElosua commented 2 years ago

Hi @LiNk-NY ,

We had discussed setting S4 methods with @HelenaLC and have revisited the conversation. We agree that plotImage should not be a an S4 Method since it's name can easily overlap with a function name in Seurat, SpatialExperiment, and SingleCellExperiment. However, for the SPOTlight specific functions discussed above -- trainNMF, runDeconvolution, and SPOTlight we followed practices employed by other commonly used packages in Bioconductor used to analyze Single-Cell data, like scran, scater, scuttle.

At the moment we would prefer to stick with the current design while being aware of the potential conflicts this could cause and commit to rapidly adapt the code in case these happen.

Thank you for the input and looking forward to hearing from you!

LiNk-NY commented 2 years ago

Hi Marc, @MarcElosua

Some of those classes are from the same developer e.g., SingleCellExperiment but some methods are inconsistently implemented. On the other hand, you may consider the scran::clusterCells approach.

It's best to avoid polluting the class's namespace unless you developed the original class and are implementing analysis methods in a different package (as may be the case for the packages you listed).

Best regards, Marcel

bioc-issue-bot commented 2 years ago

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

MarcElosua commented 2 years ago

Hi @LiNk-NY

Following your advice we have removed all the methods and replaced them with helper functions now found in R/utils.R. We also added unit tests for them in tests/testthat/test-utils.R. Lastly, we brushed up on some minor issues users had reported.

Thank you for ll your help, Best, Marc

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: "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/SPOTlight 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: 87534b92311aec0f254297dcb9478e4b63e9ee81

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.

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

MarcElosua commented 2 years ago

As the previous Check failed due to TimeOut I used mock data for the example of the function SPOTlight to speed it up.

LiNk-NY commented 2 years ago

Hi Marc, @MarcElosua

Thank you for making those updates! Your package has been accepted.

Best regards, Marcel

PS. As you continue to improve your package, please take care of this NOTE and use BiocCheck.

* checking R code for possible problems ... NOTE
plotSpatialScatterpie: no visible binding for global variable ‘coord_x’
plotSpatialScatterpie: no visible binding for global variable ‘coord_y’
Undefined global functions or variables:
  coord_x coord_y
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/MarcElosua.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("SPOTlight"). The package 'landing page' will be created at

https://bioconductor.org/packages/SPOTlight

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.

MarcElosua commented 2 years ago

Hi @LiNk-NY,

Thank you so much for you help and input in this process! Regarding the NOTE you mention in your last comment I think it comes from ggplot when I pass coord_x and coord_y to aes. Shoud I use aes_string in this case?

Thank you! Marc

LiNk-NY commented 2 years ago

Hi @MarcElosua

Yes, you should use the programmatic interface or you could also just use utils::globalVariables.

Best, Marcel