Bioconductor / Contributions

Contribute Packages to Bioconductor
135 stars 33 forks source link

stJoincount #2610

Closed Nina-Song closed 2 years ago

Nina-Song 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 @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="songjiar@usc.edu",role=c("cre","aut")),
    person("Rania", "Bassiouni", email="rbassiou@usc.edu", role=c("aut")),
    person("David","Craig",email="davidwcr@usc.edu", 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
vjcitn commented 2 years ago

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

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

That doesn't exist.

Nina-Song commented 2 years ago

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

vjcitn commented 2 years ago

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

lshep commented 2 years ago

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

vjcitn commented 2 years ago

OK. Ignore my comment. Thanks!

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, 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/stJoincount 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: 39f7267a8be7c0442a03407e01631d6136097853

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, 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 git@git.bioconductor.org: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 commented 2 years ago

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

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/stJoincount 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: 2823dd6accd6e674330bd2c6b620c1efdebf7554

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

lazappi commented 2 years ago

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

DESCRIPTION file

NAMESPACE file

The NEWS file

Package data

Documentation

Vignette

Man pages

Unit tests

Code

R

Nina-Song commented 2 years ago

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

lazappi commented 2 years ago

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.

lshep commented 2 years ago

@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?

Nina-Song commented 2 years ago

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 commented 2 years ago

Hi @lazappi,

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

Thanks for your help!

Nina

lazappi commented 2 years ago
  • 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.

bioc-issue-bot commented 2 years ago

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

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/stJoincount 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: 5b1f6c67a71879d9535fd2d59232d8a8e522e911

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/stJoincount 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: 356ae42de2c4814cb25d89eb10c996a06d52a07c

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

lazappi commented 2 years ago

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.

Nina-Song commented 2 years ago

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

bioc-issue-bot commented 2 years ago

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

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/stJoincount 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: 7752307259402848aaba344552af9ce7cb5e9f77

Nina-Song commented 2 years ago

Hi @lazappi,

Thank you again for your patience and valuable advice! The following changes have been made in each category -

DESCRIPTION file 🟢 Added Software to BiocViews 🟢 Added a BugReports field and a URL field 🟢 Added ORCiD

NAMESPACE file 🟢 Selectively import functions using importFrom instead of import all with import

The NEWS file 🟢 Changed the format of NEWS file

Package data 🟢 Internal data in the inst/extdata/ directory has been documented in inst/script

Vignette 🟢 Followed the BiocStyle package for formatting 🟢 Added an Introduction section and an Installation section 🟢 Presented the workflow as well as the functionality of the package 🟢 Set warnings = TRUE

Unit tests 🟢 Added unit tests to the package that cover all exported functions.

R code 🟢 Majority of those issues have been addressed. 🔵 The input format now has been changed to a CSV file, which won't affect all of the major analyses. Therefore, previous issues such as rhdf5, BiocFileCache, or using Seurat vs. SpatialExperiment objects were no longer there.

Thank you again!

Sincerely, Nina

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

lazappi commented 2 years ago

🟢 Changed the format of NEWS file

You don't need the ```markdown code formatting. I'm also not sure what will happen with the ------- lines when this is read by the Bioconductor release system.

🟢 Internal data in the inst/extdata/ directory has been documented in inst/script

I couldn't find any documentation here. This should be a text/R file that describes where the data in /inst/extdata comes from, not an .Rda file.

There doesn't actually seem to be any data in inst/extdata anymore, just some PNG files? I'm not sure what changed here or what these images are for.

🟢 Added an Introduction section and an Installation section

The installation section should go at the start of the vignette.

I also noticed a few typos. You can check for these using the {spellcheck} package.

Unit tests 🟢 Added unit tests to the package that cover all exported functions.

I find the checkInput() functions in your tests a bit strange, particularly as the aren't reused, but I think it's fine if that works for you.

R code 🟢 Majority of those issues have been addressed.

🔵 The input format now has been changed to a CSV file, which won't affect all of the major analyses. Therefore, previous issues such as rhdf5, BiocFileCache, or using Seurat vs. SpatialExperiment objects were no longer there.

  • For the part of "getter or setter methods", there was one @ access in the previous version of this package, clusterName@data@values. clusterName is a S4 Class RasterLayer.
  • After adding setGeneric("getValues", function(x){standardGeneric("getValues")}) setMethod("getValues", c(x = "RasterLayer"),function(x){return(x@data@values)}) , it gave an error "cannot add bindings to a locked environment". I am not sure if I understood the term of getter and setter correctly.

If this object is defined in another package then you won't be able to add getter/setter functions yourself. In this case, if they aren't provided by the package authors then you have no choice but to use @ or slot() (but if they are provided please use them).

bioc-issue-bot commented 2 years ago

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

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/stJoincount 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: 44dbb6c5b151a5e20115e5ef1a1fd89a25d1338a

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

Nina-Song commented 2 years ago

Hi @lazappi ,

Thank you again for your valuable feedback! The following changes have been made:

Thank you again!

Sincerely, Nina

lazappi commented 2 years ago
  • [ ] Changed the format of NEWS file

I think there is still one ---------- line in there

  • [ ] Added text description of extdata in inst/script

There is still some information missing here, mostly links to the source for the dataset (GEO, publication etc.). Ideally this would be an R script that shows where you got the data from and how it was processed.

[ ] Added sections in the vignette showing how to extract the relevant information from Seurat/SpatialExperiment objects, and how to generate the same data.frame

These sections should be evaluated, you may need to add more example data to make this possible. This code could also do with some more comments or explanation of the steps in the text (and I still think it would be better to let users supply a Seurat/SpatialExperiment directly)

lazappi commented 2 years ago

Hi @Nina-Song

Another follow up comment, sorry. Another reviewer noticed that the data.frame format is not documented in the function documentation. Each function parameter documentation should say that the input is a data.frame and what columns etc. it must have.

They also suggested that another solution to the object problem would be to write a preparation function that creates the data.frame from a Seurat/SpatialExperiment. Then the workflow for the user would be input object -> prepared data.frame -> analysis functions. We strongly feel that letting the user use standard objects as the input to your package without requiring manual manipulation would make the package much more useable and accessible (and therefore more likely to be used).

Nina-Song commented 2 years ago

Dear @lazappi ,

Thank you so much for your valuable suggestions and prompt follow-up comment, they are very helpful.

One thing we are not sure about is, whether could we create this data preparation function without showing an example (like dummy Seurat/SpatialExperiment objects) to this one, and still keep the current external data (the data.frame in /extdata) as the example data for the rest functions? Since if adding two more objects, even if they are 'dummy', they might be larger than 5 MB; and for the rest functions, they may not be necessary to run start very beginning.

Thanks again for your time and looking forward to hearing back from you!

Sincerely, Nina

bioc-issue-bot commented 2 years ago

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

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/stJoincount 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: b01352820a922654c1db802a26ae753c40420c4f

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

Nina-Song commented 2 years ago

Dear @lazappi ,

Thank you so much for your valuable suggestions and prompt follow-up comment again!

As the last comment suggested, data preparation functions are added to make the workflow for the user from input object -> prepared data.frame -> analysis functions. Dummy examples for Seurat and SpatialExperiment objects are added to extdata. Each function parameter documentation now includes an explanation of the input data.frame requirements.

Thanks again and please let me know if there is anything else needed. We are looking forward to hearing back from you!

Best, Nina

Nina-Song commented 2 years ago

Hello @lazappi ,

Since today is the deadline for API changes, I just wanted to follow up on the review process. Thank you again for your help.

Best, Nina

lshep commented 2 years ago

@Nina-Song the API deadline is mostly for package already accepted in Bioconductor. Since there is a lot of interdependent packages we encourage the API deadline to prevent last minute changes having a cascading effect to break packages dependent on it. You'll still have until the 26 to be accepted.