Contributions icon indicating copy to clipboard operation
Contributions copied to clipboard

stJoincount

Open Nina-Song opened this issue 3 years ago • 29 comments
trafficstars

Update the following URL to point to the GitHub repository of the package you wish to submit to Bioconductor

  • Repository: https://github.com/Nina-Song/stJoincount

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 Bioconductor Package Naming Policy and acknowledge Bioconductor may retain use of package name.

  • [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.

  • [x] I am familiar with the Bioconductor code of conduct and agree to abide by it.

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

Nina-Song avatar Apr 01 '22 11:04 Nina-Song

Hi @Nina-Song

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: stJoincount
Type: Package
Title: stJoincount - Join count statistic for quantifying spatial correlation between clusters
Version: 0.99.0
Authors@R: c(
    person("Jiarong", "Song", email="[email protected]",role=c("cre","aut")),
    person("Rania", "Bassiouni", email="[email protected]", role=c("aut")),
    person("David","Craig",email="[email protected]", role=c("aut"))
    )
Description: stJoincount, a bioinformatics tool uses join count statistic for quantifying spatial correlation between clusters.
    This tool requires pre-processed spatial transcriptomics data, and either with user pre-defined clusters or standard louvain clusters from Cell Ranger outputs.
    The first part of the algorithm is rasterization of samples for join count analysis. It rasterize each cluster with calculated extent and resolution, then merge to form a large mosaic.
    The second part of the algorithm is to preform join count analysis on the integrated mosaic to quantify spatial correlation between clusters.
License: MIT + file LICENSE
Encoding: UTF-8
Depends:
    R (>= 4.1)
Imports: Seurat,
    utils, 
    graphics, 
    stats, 
    raster, 
    spdep, 
    sp, 
    reshape2, 
    pheatmap
LazyData: FALSE
RoxygenNote: 7.1.2
Suggests: 
    knitr,
    rmarkdown
VignetteBuilder: knitr
biocViews: Transcriptomics, Clustering

bioc-issue-bot avatar Apr 01 '22 11:04 bioc-issue-bot

Do you check your package before submitting, on a "clean" system? Vignette:

```{r setup, message = FALSE, warning=FALSE}
library(joincount)

That doesn't exist.

vjcitn avatar Apr 03 '22 17:04 vjcitn

Dear @vjcitn ,

My apology for the mistake, "joincount" is the package that I built for testing. I restart the R session and now it passed the R cmd check.

During the R cmd check, there is one warning that I could not fix for a while. checking data for non-ASCII characters (1.3s) Error loading dataset 'humanBC': Error in .requirePackage(package) : unable to find required package 'Seurat'

 The dataset(s) may use package(s) not declared in Depends/Imports.

I have tried putting "Seurat" in either Depends or imports, but none of them get this warning sign fixed. "#' @ import Seurat" is also present at the beginning of each function that needs this package.

Thanks in advance!

Nina

Nina-Song avatar Apr 03 '22 18:04 Nina-Song

Thanks for your reply. I will pass the package for review. But this should not be in the vignette:

bfc <- BiocFileCache::BiocFileCache()
bc.url <-
  paste0(
    "https://cf.10xgenomics.com/samples/spatial-exp/",
    "1.1.0/V1_Breast_Cancer_Block_A_Section_1/",
    c(
      "V1_Breast_Cancer_Block_A_Section_1_filtered_feature_bc_matrix.tar.gz",
      "V1_Breast_Cancer_Block_A_Section_1_spatial.tar.gz",
      "V1_Breast_Cancer_Block_A_Section_1_analysis.tar.gz"
    )
  )


h5.url <-
  paste0(
    "https://cf.10xgenomics.com/samples/spatial-exp/",
    "1.1.0/V1_Breast_Cancer_Block_A_Section_1/",
    "V1_Breast_Cancer_Block_A_Section_1_filtered_feature_bc_matrix.h5"
  )

We have no guarantees that 10xgenomics will maintain this. The data should be added to ExperimentHub. @lshep

vjcitn avatar Apr 16 '22 10:04 vjcitn

@vjcitn It's up for debate. Since they are caching and the 10xgenomics site seems to be well maintained public entity, I'm inclined to leave as is since they have implemented the caching mechanism. Just make sure that the caching is set up/working properly. But @vjcitn will have the final say in this matter.

lshep avatar Apr 18 '22 11:04 lshep

OK. Ignore my comment. Thanks!

vjcitn avatar Apr 18 '22 13:04 vjcitn

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 avatar Apr 18 '22 14:04 bioc-issue-bot

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 [email protected]:packages/stJoincount to trigger a new build. A quick tutorial for setting up remotes and pushing to upstream can be found here.

bioc-issue-bot avatar Apr 18 '22 15:04 bioc-issue-bot

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

bioc-issue-bot avatar Apr 19 '22 21:04 bioc-issue-bot

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, TIMEOUT, 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 [email protected]:packages/stJoincount to trigger a new build. A quick tutorial for setting up remotes and pushing to upstream can be found here.

bioc-issue-bot avatar Apr 19 '22 21:04 bioc-issue-bot

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

bioc-issue-bot avatar Apr 19 '22 22:04 bioc-issue-bot

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 [email protected]:packages/stJoincount to trigger a new build. A quick tutorial for setting up remotes and pushing to upstream can be found here.

bioc-issue-bot avatar Apr 19 '22 22:04 bioc-issue-bot

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

bioc-issue-bot avatar Apr 20 '22 04:04 bioc-issue-bot

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 [email protected]:packages/stJoincount to trigger a new build. A quick tutorial for setting up remotes and pushing to upstream can be found here.

bioc-issue-bot avatar Apr 20 '22 04:04 bioc-issue-bot

Hi @Nina-Song

Thanks for submitting stJoincount :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

  • [ ] :rotating_light: Please address as many of the warnings and notes from BiocCheck::BiocCheck() in the build report as possible

DESCRIPTION file

  • [ ] :warning: It is recommended to add ORCiDs to the Authors@R field
  • [ ] :rotating_light: Bioconductor packages should use the Bioconductor {rhdf5} package rather than the CRAN {hdf5r} package
  • [ ] :green_circle: You may want to add Software to BiocViews
  • [ ] :warning: It is recommended to add a BugReports field, this is usually the URL for your GitHub issues page or the Bioconductor support forum
  • [ ] :warning: It is recommended to add a URL field, this is usually the URL for your GitHub repository or another package website

NAMESPACE file

  • [ ] :rotating_light: Please selectively import functions using importFrom instead of import all with import

The NEWS file

  • [ ] :rotating_light: Please check the format of your NEWS file is correct, see here for more info

Package data

  • [ ] :rotating_light: Internal data in the inst/extdata/ directory should be documented in inst/script
  • [ ] :rotating_light: BiocFileCache should be used for downloaded data. I think you have done this for most things but not the .h5 file in the vignette.
  • [ ] :warning: If you are using the data in your examples it may be better to export it

Documentation

  • [ ] :green_circle: I noticed typos in some places, you can check for these using the {spellcheck} package or usethis::use_spellcheck()

Vignette

  • [ ] :rotating_light: Vignettes should use the BiocStyle package for formatting
  • [ ] :rotating_light: Please add a table of contents to the vignette
  • [ ] :rotating_light: Please add an Introduction section
  • [ ] :rotating_light: Please add an Installation section, this should include the Bioconductor installation instructions with eval = FALSE
  • [ ] :rotating_light: The vignette should be more than just a list of functions. It should describe the functionality of the package and demonstrate how they can be used together to perform a task. Please add more detail.
  • [ ] :rotating_light: In general vignettes should use warnings = TRUE so that the output is as close as possible to what users will see when they run the code.

Man pages

  • [ ] :rotating_light: In general your function documentation is very brief, please add more detail about what the functions do
  • [ ] :warning: It is recommended to add a package man page

Unit tests

  • [ ] :rotating_light: Please add unit tests to the package. These should cover all exported functions.

Code

R

  • [ ] :rotating_light: Please use vapply instead of sapply
  • [ ] :rotating_light: Please use seq_along()/seq_len() instead of 1:...
  • [ ] :rotating_light: Please access data in objects using getter or setter methods, not directly with @ or slot()
  • [ ] :warning: Check you are consistent in using <- rather than =, I think I noticed some = in the vignette
  • [ ] :rotating_light: Please usemessage(), warning(), stop() instead of cat() or print()
  • [ ] :rotating_light: The {Seurat} package is not part of Bioconductor. While it is ok to support Seurat objects your package should also support equivalent Bioconductor objects, in this case probably SpatialExperiment or one of the classes it inherits from (SingleCellExperiment/SummarizedExperiment)
  • [ ] :rotating_light: The documentation for function arguments is brief, please expand this
  • [ ] :warning: Consider adding validity checks for function arguments
  • [ ] :warning: Please check that all your for loops are necessary
  • [ ] :warning: Please remove any commented code that is not used

lazappi avatar Apr 20 '22 14:04 lazappi

Hi @lazappi ,

Thanks a lot for your feedback and these valuable comments. I will make adjustments as soon as possible.

I am wondering if there is a deadline for developers to make changes before getting accepted?

Thanks again for your help. @lazappi

Sincerely, Nina

Nina-Song avatar Apr 20 '22 21:04 Nina-Song

You can find the release schedule here https://bioconductor.org/developers/release-schedule/. You may have just missed the deadline for acceptance already but @Bioconductor/core can confirm.

lazappi avatar Apr 21 '22 07:04 lazappi

@Nina-Song May we expect updates soon? We like to see progress in a 3-4 week time frame to keep the review process moving?

lshep avatar May 09 '22 13:05 lshep

Hi @lshep ,

Thank you for following up! I am working on those issues, and should have them down by this week. I will keep you posted.

Thanks again!

Nina

Nina-Song avatar May 09 '22 15:05 Nina-Song

Hi @lazappi,

My apology for the delay. The majority of the comments have been addressed, there are some minor questions that remained:

  • The package is built based on the Seurat object. The majority of functions use the Seurat objects as the input. If we would like to support the SpatialExperiment object, does it refers to writing separate functions which do the same thing but use the SpatialExperiment object as the input?

  • While defining a vignette is to demonstrate how users can be used together to perform a task, should we only present and explain those commands in the main workflow?

Thanks for your help!

Nina

Nina-Song avatar May 17 '22 08:05 Nina-Song

  • The package is built based on the Seurat object. The majority of functions use the Seurat objects as the input. If we would like to support the SpatialExperiment object, does it refers to writing separate functions which do the same thing but use the SpatialExperiment object as the input?

Yes, but you should avoid duplicating code. Usually the best approach is to have one function which extracts the required information from the supported objects and passes it to another function which does the internal processing. This can be done something like this:

my_func <- function(x) {
	if (is(x, "Seurat") {
		x <- ...
	}

	if (is(x, "SpatialExperiment") {
		x <- ...
    }

	.my_func_internal(x)
}

.my_func_internal <- function(x) {
	# Do the processing here
}

It's a little bit more work but a better way is to S3 dispatch with a method for each of the supported objects and a default method which does the processing.

 my_func <- function (x, ...) {
   UseMethod("my_func", x)
 }

my_func.Seurat <- function(x, ...) {
	x <- ...
	my_func(x, ...)
}

my_func.SpatialExperiment <- function(x, ...) {
	x <- ...
	my_func(x, ...)
}

my_func.default <- function(x, ...) {
	# Do the processing here
}

Hopefully that makes sense but let me know if it's not clear.

  • While defining a vignette is to demonstrate how users can be used together to perform a task, should we only present and explain those commands in the main workflow?

Yes, you don't need to cover every function or function argument but you should cover the main things. It's also useful to include things like how a plot should be interpreted or when a function argument should be used to how to choose a value for things that need to be set manually.

lazappi avatar May 18 '22 06:05 lazappi

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

bioc-issue-bot avatar Aug 22 '22 07:08 bioc-issue-bot

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 [email protected]:packages/stJoincount to trigger a new build. A quick tutorial for setting up remotes and pushing to upstream can be found here.

bioc-issue-bot avatar Aug 22 '22 07:08 bioc-issue-bot

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

bioc-issue-bot avatar Aug 22 '22 07:08 bioc-issue-bot

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 [email protected]:packages/stJoincount to trigger a new build. A quick tutorial for setting up remotes and pushing to upstream can be found here.

bioc-issue-bot avatar Aug 22 '22 07:08 bioc-issue-bot

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

bioc-issue-bot avatar Aug 22 '22 20:08 bioc-issue-bot

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 [email protected]:packages/stJoincount to trigger a new build. A quick tutorial for setting up remotes and pushing to upstream can be found here.

bioc-issue-bot avatar Aug 22 '22 21:08 bioc-issue-bot

Hi @Nina-Song. It looks like the build is running without any issue now which is great. Please comment with the changes you have made when you are ready for me to review the package again.

lazappi avatar Aug 26 '22 06:08 lazappi

Hi @lazappi,

Thank you so much for your last advice. The input format now has been changed to a CSV file, which won't affect all of the major analyses, as well as avoids using Seurat or SpatialExperiment objects (which could be quite large and unnecessarily time-consuming for this statistical analysis pipeline). Currently, I am working on adding unit tests for functions. Once this step gets done, the package will be ready to under review.

Thank you again for your advice and patience!

Best, Nina

Nina-Song avatar Aug 29 '22 22:08 Nina-Song

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

bioc-issue-bot avatar Sep 06 '22 23:09 bioc-issue-bot