Bioconductor / Contributions

Contribute Packages to Bioconductor
135 stars 33 forks source link

SpaNorm #3580

Closed bhuvad closed 2 weeks ago

bhuvad commented 1 month 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 1 month ago

Hi @bhuvad

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: SpaNorm
Title: Spatially-aware normalisation for spatial transcriptomics data
Version: 0.99.0
Authors@R: c(
  person(given = "Agus",
        family = "Salim",
        role = c("aut"),
        email = "salim.a@unimelb.edu.au",
        comment = c(ORCID = "0000-0003-3999-7701")),
  person(given = "Dharmesh D.",
         family = "Bhuva",
         role = c("aut", "cre"),
         email = "dharmesh.bhuva@adelaide.edu.au",
         comment = c(ORCID = "0000-0002-6398-9157"))
  )
Description: This package implements the spatially aware library size normalisation algorithm, SpaNorm. SpaNorm normalises out library size effects while retaining biology through the modelling of smooth functions for each effect. Normalisation is performed in a gene- and cell-/spot- specific manner, yielding library size adjusted data.
License: GPL (>= 3)
Encoding: UTF-8
LazyDataCompression: bzip2
Roxygen: list(markdown = TRUE)
RoxygenNote: 7.3.1
biocViews: Software, GeneExpression, Transcriptomics, Spatial
Depends:
  R (>= 4.4)
Imports:
    edgeR,
    Matrix,
    matrixStats,
    methods,
    scran,
    Seurat,
    SeuratObject,
    SingleCellExperiment,
    SpatialExperiment,
    stats,
    SummarizedExperiment,
    S4Vectors,
    utils
Suggests:
    testthat (>= 3.0.0),
    knitr,
    rmarkdown,
    prettydoc,
    pkgdown,
    covr,
    BiocStyle,
    ggplot2,
    patchwork
URL: https://bhuvad.github.io/SpaNorm
BugReports: https://github.com/bhuvad/SpaNorm/issues
VignetteBuilder: knitr
Config/testthat/edition: 3
LazyData: true
vjcitn commented 1 month ago

Nice package, I had a preliminary look. What is the principle of subsampling with fraction sample.p in SpaNorm? Is it a purely random sample of spots, or is there stratification, perhaps by region and/or sequencing depth?

bhuvad commented 1 month ago

Hi Vince,

At the moment it is purely random and serves the purpose of evenly sampling the tissue region to obtain an unbiased approximation across space. However, we may provide other sampling strategies to sample dense regions more than sparse regions. The code in the package will easily allow such an extension as we did think of this during the design phase. If you think of examples to motivate such an approach over an unbiased sampling across space, please do share them so that we can prioritise this among our other planned works.

Cheers, Dharmesh

bioc-issue-bot commented 1 month ago

Your package has been added to git.bioconductor.org to continue the pre-review process. A build report will be posted shortly. Please fix any ERROR and WARNING in the build report before a reviewer is assigned or provide a justification on why you feel the ERROR or WARNING should be granted an exception.

IMPORTANT: Please read this documentation for setting up remotes to push to git.bioconductor.org. All changes should be pushed to git.bioconductor.org moving forward. It is required to push a version bump to git.bioconductor.org to trigger a new build report.

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 1 month ago

Dear Package contributor,

This is the automated single package builder at bioconductor.org.

Your package has been built on the Bioconductor Single Package Builder.

Congratulations! The package built without errors or warnings on all platforms.

Please see the build report for more details.

The following are build products from R CMD build on the Single Package Builder: Linux (Ubuntu 24.04.1 LTS): SpaNorm_0.99.0.tar.gz

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

bioc-issue-bot commented 1 month ago

A reviewer has been assigned to your package for an indepth review. Please respond accordingly to any further comments from the reviewer.

lshep commented 1 month ago

I apologize for the delay. I should have a review for you in the next few days.

bhuvad commented 1 month ago

I understand you will have a pretty intense workload right now so all good Lori.

lshep commented 4 weeks ago

Thank you for your understanding. Please find comments below:

> p_region = spatialCoords(HumanDLPFC) |>
  as.data.frame() |>
  cbind("Region" = HumanDLPFC$AnnotatedCluster) |>
  ggplot(aes(pxl_col_in_fullres, pxl_row_in_fullres, colour = Region)) +
  geom_point() +
  scale_colour_brewer(palette = "Paired", guide = guide_legend(override.aes = list(shape = 15, size = 5))) +
  labs(title = "Region") +
  custom_theme()
p_region
Error in custom_theme() : could not find function "custom_theme"

and then

> logcounts(HumanDLPFC) = log2(counts(HumanDLPFC) + 1)
p_counts = plotGeneExpression(HumanDLPFC, "MOBP") +
  ggtitle("Counts")
p_region + p_counts
Error in plotGeneExpression(HumanDLPFC, "MOBP") : 
  could not find function "plotGeneExpression"

You seem to have hidden code from the user that is necessary for us to reproduce your vignette. These should probably be included as functions in your package so the user doens't need to manually input them.

bioc-issue-bot commented 3 weeks ago

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

bioc-issue-bot commented 3 weeks ago

Dear Package contributor,

This is the automated single package builder at bioconductor.org.

Your package has been built on the Bioconductor Single Package Builder.

Congratulations! The package built without errors or warnings on all platforms.

Please see the build report for more details.

The following are build products from R CMD build on the Single Package Builder: Linux (Ubuntu 24.04.1 LTS): SpaNorm_0.99.1.tar.gz

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

bhuvad commented 3 weeks ago

Hi Lori,

Thank you for taking the time to review my package and for providing such useful feedback to improve the code quality. As most members of the community will agree, we appreciate the time and effort the core team puts into maintaining and enhancing the quality of Bioconductor. I have taken your suggestions on board and have made changes to the package as enumerated below.

I have removed the redundant .gitignore file as well as the Rproj filewhich serves no purpose. However, the pkgdown, Dockerfile, and devcontainer files are important to retain on our github branch and will therefore also be passed on to the devel branch for Bioconductor. These files are used to create a reproducible development environment for any developer willing to contribute to this project in the future. If I temporarily add them to the .gitignore, push, and re-add them, I would have to make sure that this is done each time prior to an update being submitted to the devel branch. These files are already in the .Rbuildignore so will not be an issue elsewhere and they are very lightweight as well. As such, would it be ok to leave these on the repo due to its value to developers?

I have added the file as requested

Most of these packages are used through explicit function calls (examples enumerated for each below) and will therefore not be in the NAMESPACE file.

  1. edgeR::estimateDisp
  2. Matrix::colSums
  3. matrixStats::colMedians
  4. scran::quickCluster
  5. SeuratObject::LayerData
  6. SingleCellExperiment::sizeFactors
  7. SpatialExperiment::spatialCoords
  8. SummarizedExperiment::assay
  9. S4Vectors::metadata

I have fixed the following notes:

  1. Switched LazyData to false in the DESCRIPTION file.
  2. Added CITATION file.
  3. Fixed issue with global variables/functions W and str. The former was a typo (should be Wsub) that has been fixed and the latter was a missing NAMESPACE definition (which I use the explicit call for, i.e., utils::str).
  4. Moved Seurat from imports to suggests as it is currently only used for tests. Future functionality may require it at which point I will move it back.
  5. Added CellBiology to biocViews for this package as it is appropriate.
  6. _Changed 1:... to `seqlen(...).
  7. Changed dontrun to donttest.
  8. Majority of the long line issues are from documentation and vignettes. The rest cannot be helped as these are mathematical operations that are difficult to read if broken into multiple lines.
  9. The long functions (>50 lines) cannot be modularised further in any meaningful way. Where this was possible, I had done that.

Thank you for bringing this into my attention. I did get a similar query from a user, and I realise now that users are likely to reproduce what they see in the report rather than look at the vignette code. This is what produces the error. I have now added a new function called plotSpatial() to the package that is used in the vignettes. This function is a lot more powerful and can be used to plot gene expression, cell/spot annotation, or reduced dimensions. All code is now visible and reproducible from the vignettes.

I have performed a spell check on the package and have fixed up spelling errors in the vignette and documentation.

Please let me know if you need me to make any other changes to the package.

Cheers, Dharmesh

lshep commented 3 weeks ago

Note: The dockerfile will be out-of-date unless updated each release in its current state; if the intention is to have this as a developer tools it might be worth using the devel docker image rather than tie to a specific release.
Would we be able to meet half way with the Dockerfile and at least move into a inst/scripts/ folder rather than the top level directory?

bioc-issue-bot commented 3 weeks ago

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

bioc-issue-bot commented 3 weeks ago

Dear Package contributor,

This is the automated single package builder at bioconductor.org.

Your package has been built on the Bioconductor Single Package Builder.

Congratulations! The package built without errors or warnings on all platforms.

Please see the build report for more details.

The following are build products from R CMD build on the Single Package Builder: Linux (Ubuntu 24.04.1 LTS): SpaNorm_0.99.2.tar.gz

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

bhuvad commented 3 weeks ago

Those are really good suggestions. I have swapped out the container used. I thought I was using devel but might have forgotten to swap it so thanks for picking that up. I also figured it will be easier to move it to the .devcontainer folder which hosts the other configurations to build the development environment. This was, all code related to the developer tool sit within this one folder. Please let me know if there are issues with this and I will address it as per your guidance.

lshep commented 2 weeks ago

Thank you. I think this is a happy medium. Thanks for the changes.

bioc-issue-bot commented 2 weeks 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 weeks ago

The default branch of your GitHub repository has been added to Bioconductor's git repository as branch devel.

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

https://bioconductor.org/packages/SpaNorm

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.