Contributions icon indicating copy to clipboard operation
Contributions copied to clipboard

submitting new package CatsCradle

Open michaeldshapiro opened this issue 1 year ago • 6 comments

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

  • Repository: https://github.com/annaladdach/CatsCradle

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.

michaeldshapiro avatar Jun 04 '24 08:06 michaeldshapiro

Hi @michaeldshapiro

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: CatsCradle
Title: This package discovers gene clusters in Seurat data
Version: 0.99.0
Authors@R:
    person(given = "Anna",
        family = "Laddach",
        role = c("aut", "cre"),
        email = "[email protected]",
        comment = c(ORCID = "0000-0001-5552-6534"))
    person(given = "Michael",
        family = "Shapiro",
        role = c("aut", "cre"),
        email = "[email protected]",
        comment = c(ORCID = "0000-0002-2769-9320"))
Description: The basic insight driving this package is the idea that by transposing the expression matrix of a Seurat object, we can use the usual Seurat technology to cluster genes and to visualise their relationships in two dimensions, e.g., as UMAP plots.  In addition we are then able to use the nearest neighbor graph to gain additional insight into specific genes by querying their neighborhood in this graph. 
License: MIT + file LICENSE
Encoding: UTF-8
Roxygen: list(markdown = TRUE)
RoxygenNote: 7.3.1
Imports: Seurat,
	 ggplot2,
	 networkD3,
	 stringr,
	 pracma,
	 knitr,
	 reshape2,
	 rdist,
	 igraph,
	 interp,
	 geometry,
	 Rfast,
	 dplyr,
	 data.table,
	 abind,
	 pheatmap,
	 fossil
Depends: 
    R (>= 4.0)
LazyData: true
VignetteBuilder: knitr
LazyDataCompression: xz
biocViews:
    BiologicalQuestion,
    StatisticalMethod,
    GeneExpression,
    SingleCell,
    Transcriptomics

bioc-issue-bot avatar Jun 04 '24 08:06 bioc-issue-bot

The docs directory should be removed. These documentations should be generated automatically with R CMD build.

Seurat objects are not Bioconductor objects. This package should be altered to additionally be able use the main Bioconductor object for expression matrix, SummarizedExperiment/SingleCellExperiment.

Please also be sure to run R CMD build, R CMD check and BiocCheck on the package.

> cellTypesPerCellType = computeCellTypesPerCellTypeMatrix(NBHDByCTMatrix,
+                                                      smallXenium$seurat_clusters)
Error in `x[[i, drop = TRUE]]`:
! ‘seurat_clusters’ not found in this Seurat object
 
Backtrace:
     ▆
  1. ├─CatsCradle::computeCellTypesPerCellTypeMatrix(...)
  2. │ ├─stats::aggregate(nbhdByCellType, list(cellTypes), sum)
  3. │ └─stats::aggregate.data.frame(...)
  4. ├─smallXenium$seurat_clusters
  5. └─SeuratObject:::`$.Seurat`(smallXenium, seurat_clusters)
  6.   ├─x[[i, drop = TRUE]]
  7.   └─SeuratObject:::`[[.Seurat`(x, i, drop = TRUE)
  8.     └─base::tryCatch(...)
  9.       └─base (local) tryCatchList(expr, classes, parentenv, handlers)
 10.         └─base (local) tryCatchOne(expr, names, parentenv, handlers[[1L]])
 11.           └─value[[3L]](cond)
 12.             └─rlang::abort(...)

lshep avatar Jun 07 '24 15:06 lshep

We have pushed a new version of the package with the updated version number 0.99.1. We believe this version responds successfully to the suggestions. In particular, it now passes R CMD check and BiocCheck, and we have removed to doc folder.

We like the idea of enabling the package to work with, e.g., SingleCellExperiment objects. Functions which accept Seurat objects now also accept equivalent data in the form of a SingleCellExperiment of SpatialExperiment object. Similarly, functions which return as Seurat object can now return a SingleCellExperiment of SpatialExperiment at the user's request. This has been accomplished using adaptors that are only slightly deeper than, e.g., as.SingleCellExperiment. We have also provided expamples showing this capability.

We think this raises a larger question for the community. Seurat is an industry standard and will continue to evolve. We wonder if there oughtn't be a package which provides deeper conversions between Seurat and Bioconductor objects. This is beyond the scoper of CatsCradle and deserves to be treated as its own project.

Many thanks for your review of our work.

michaeldshapiro avatar Jun 26 '24 10:06 michaeldshapiro

Agreed regarding Seurat/Bioc object conversion. https://satijalab.org/seurat/archive/v4.3/conversion_vignette looks like there was at least some effort to make some standardized conversions between objects; however I don't know how mature it is or if a deeper effort is being investigated.

lshep avatar Jul 01 '24 12:07 lshep

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 avatar Jul 01 '24 14:07 bioc-issue-bot

Dear Package contributor,

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

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

On one or more platforms, the build results were: "WARNINGS, 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.

The following are build products from R CMD build on the Single Package Builder: Linux (Ubuntu 22.04.3 LTS): CatsCradle_0.99.1.tar.gz macOS 12.7.1 Monterey: CatsCradle_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 [email protected]:packages/CatsCradle to trigger a new build. A quick tutorial for setting up remotes and pushing to upstream can be found here.

bioc-issue-bot avatar Jul 01 '24 14:07 bioc-issue-bot

We believe we have resolved the build issues and have pushed a new version and bumped the version number.

One of the issues involved the size of the tarball. We found that bringing the tarball down under the 5M limit required severe sugery on one of our data objects. Single cell data can be large. In particular, getting the tarball down to size required removing enough features that this restricted some of the documentation we can provide. We wonder if it isn't time to increase the size limit on tarballs and wonder where is the best place to raise this issue.

michaeldshapiro avatar Jul 16 '24 13:07 michaeldshapiro

We pushed a new version with a new version number almost a week ago, but this does not seem to have triggered a new build. Accordingly, we have pushed a new version with increased version number. Is there something different we should be doing to trigger a build?

michaeldshapiro avatar Jul 22 '24 13:07 michaeldshapiro

It looks like you haven't pushed those recent updates to [email protected]:packages/CatsCradle but only to your GitHub-hosted repo?

% git clone [email protected]:packages/CatsCradle
Cloning into 'CatsCradle'...
Enter passphrase for key '/Users/peter/.ssh/id_ed25519': 
remote: Enumerating objects: 2010, done.
remote: Counting objects: 100% (2010/2010), done.
remote: Compressing objects: 100% (1006/1006), done.
remote: Total 2010 (delta 976), reused 2010 (delta 976), pack-reused 0
Receiving objects: 100% (2010/2010), 406.38 MiB | 5.89 MiB/s, done.
Resolving deltas: 100% (976/976), done.
% git log -n 1
commit 627d28528b6c5bc563ab35b9066fc3d52ecb7f20 (HEAD -> devel, origin/devel, origin/HEAD)
Author: AnnaLaddach <[email protected]>
Date:   Wed Jun 26 11:14:14 2024 +0100

    S and S transpose fix

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.

PeteHaitch avatar Jul 22 '24 21:07 PeteHaitch

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

bioc-issue-bot avatar Jul 23 '24 09:07 bioc-issue-bot

Dear Package contributor,

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

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

On one or more platforms, the build results were: "WARNINGS, 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.

The following are build products from R CMD build on the Single Package Builder: macOS 12.7.1 Monterey: CatsCradle_0.99.4.tar.gz Linux (Ubuntu 22.04.3 LTS): CatsCradle_0.99.4.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 [email protected]:packages/CatsCradle to trigger a new build. A quick tutorial for setting up remotes and pushing to upstream can be found here.

bioc-issue-bot avatar Jul 23 '24 10:07 bioc-issue-bot

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

bioc-issue-bot avatar Jul 23 '24 11:07 bioc-issue-bot

Dear Package contributor,

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

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

On one or more platforms, the build results were: "WARNINGS, 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.

The following are build products from R CMD build on the Single Package Builder: macOS 12.7.1 Monterey: CatsCradle_0.99.5.tar.gz Linux (Ubuntu 22.04.3 LTS): CatsCradle_0.99.5.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 [email protected]:packages/CatsCradle to trigger a new build. A quick tutorial for setting up remotes and pushing to upstream can be found here.

bioc-issue-bot avatar Jul 23 '24 11:07 bioc-issue-bot

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

bioc-issue-bot avatar Jul 23 '24 13:07 bioc-issue-bot

Dear Package contributor,

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

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

On one or more platforms, the build results were: "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.

The following are build products from R CMD build on the Single Package Builder: ERROR before build products produced.

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

bioc-issue-bot avatar Jul 23 '24 13:07 bioc-issue-bot

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

bioc-issue-bot avatar Jul 23 '24 15:07 bioc-issue-bot

Dear Package contributor,

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

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

On one or more platforms, the build results were: "WARNINGS, 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.

The following are build products from R CMD build on the Single Package Builder: macOS 12.7.1 Monterey: CatsCradle_0.99.7.tar.gz Linux (Ubuntu 22.04.3 LTS): CatsCradle_0.99.7.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 [email protected]:packages/CatsCradle to trigger a new build. A quick tutorial for setting up remotes and pushing to upstream can be found here.

bioc-issue-bot avatar Jul 23 '24 15:07 bioc-issue-bot

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

bioc-issue-bot avatar Jul 24 '24 10:07 bioc-issue-bot

Dear Package contributor,

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

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

On one or more platforms, the build results were: "WARNINGS, 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.

The following are build products from R CMD build on the Single Package Builder: macOS 12.7.1 Monterey: CatsCradle_0.99.8.tar.gz Linux (Ubuntu 22.04.3 LTS): CatsCradle_0.99.8.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 [email protected]:packages/CatsCradle to trigger a new build. A quick tutorial for setting up remotes and pushing to upstream can be found here.

bioc-issue-bot avatar Jul 24 '24 10:07 bioc-issue-bot

We need some help understanding an error message in the build report. This occurs in the BiocCheck section on coding practices:

Error in seq.default(which(cond) - length(notLookback), which(cond)) : 'from' must be of length 1

We have replaced all occurences of the form 1:N with seq_len(N) and all occurences of the form 2:N with seq(from=2,to=N), etc, Furthermore, we do not see this error when running BiocCheck() locally.

Any help would be most appreciated.

michaeldshapiro avatar Jul 24 '24 10:07 michaeldshapiro

Our understanding is that the build error we are getting is due to a bug in BiocCheck. Unless someone can point to a specific problem with our code, we think it would be appropriate to move this project on to the next stage.

michaeldshapiro avatar Jul 27 '24 10:07 michaeldshapiro

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

bioc-issue-bot avatar Jul 29 '24 13:07 bioc-issue-bot

Dear Package contributor,

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

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

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.

The following are build products from R CMD build on the Single Package Builder: macOS 12.7.1 Monterey: CatsCradle_0.99.9.tar.gz Linux (Ubuntu 22.04.3 LTS): CatsCradle_0.99.9.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 [email protected]:packages/CatsCradle to trigger a new build. A quick tutorial for setting up remotes and pushing to upstream can be found here.

bioc-issue-bot avatar Jul 29 '24 13:07 bioc-issue-bot

As regards the WARNING regarding the CITATION file. We would be happy to revise this when CatsCradle is available on Bioconductor. However, the package is already being used by multiple researchers who are accessing it from github and we would like to give them something to cite.

michaeldshapiro avatar Jul 31 '24 13:07 michaeldshapiro

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

bioc-issue-bot avatar Aug 05 '24 13:08 bioc-issue-bot

main

  • [ ] All vignettes use a SeuratObject to demo capabilities. As pointed out previously, Seurat is not on Bioc. I appreciate the sentiment to assure usability across communities, however, please also include a vignette designated to using Bioc infrastructure, i.e., the SingleCellExperiment class alongside corresponding visualizations, e.g., using scater.

  • [ ] I noticed that predictAnnotation (possibly other functions) runs instantly for the SeuratObject, but stalls for the (identical) SingleCellExperiment. I traced this down to acceptor(), which calls on SCEtoSeurat, which is extremely inefficient. Granted this is exactly what is assuring interoperability with Bioc, it'd be much appreciated to assure that functions work equally on both types of objects (in terms of results and computation speed/cost).

misc

  • [ ] In the DESCRIPTION, consider adding "Spatial" to biocViews:.

  • [ ] In the README, please add installation instructions using BiocManager::install().

  • [ ] Consider adding a NEWS file to keep track of changes, in non-technical language, in the code from one release version to the next (see here).

  • [ ] Seurat has been highly volatile over the years. When depending on it, I'd suggestion specifying exactly which version is expected to be compatible with your package (i.e., Seurat (>= x.y.z)).

  • [ ] Please consider implementing unit tests; we strongly encourage them!

  • [ ] Please address build report NOTES to the best of you abilities; e.g., LazyData: in the DESCRIPTION should be set to false or removed, R version dependency should be >= 4.4.0, add URL: and BugReports: fields, add the output of the sessionInfo() at the vignette's conclusion, and more.

  • [ ] "S" is a rather unfortunate name for a package's data object (not unlikely that data(S) would overwrite the user's variable)... consider changing this to something more explicit, e.g., exSeuratObj or something.

docs

  • [ ] Data documentation is a little brief in some instances, e.g., humanLRN is missing source information or similar. Please see here for guidance, and include provenance and source information of all objects (accompanied by an inst/script/, if applicable).

  • [ ] We encourage the use of `r BiocStyle::Biocpkg("BiocStyle")` for formatting (see here).

  • [ ] I'd suggest consistently using code style for anything R-related (e.g., variable, function, class names) etc. -- if applicable, you can hyperlink external packages using BiocStyle functions, e.g., `r BiocStyle::CRANpkg("ggplot2")` and `r BiocStyle::Biocpkg("scuttle")` for packages hosted via CRAN and Bioc, respectively.

  • [ ] All package functions are currently placed in 2 scripts with 1500 lines+ This makes things very hard to grasp, review, maintain. Consider placing functionally related code/helpers in separate scripts and, if applicable, using designated, say, utils-plotting.R, utils-spatial.R etc. scripts for code used repeatedly.

code

  • [ ] In the build report, there are numerous WARNING related to namespace clashes (packages exporting identically named things). Most of these should be resolved by selective importing, which is in any case recommended; i.e., using @importFrom package function rather than @import to include a packages full namespace (see here).

  • [ ] Please also address the NOTE related to unused imports, namely, fossil,interp,knitr (the latter should go under Suggests:).

  • [ ] NOTE on "partial argument match of 'meta' to 'meta.data'" CreateSeuratObject() should be resolved with CreateSeuratObject(..., meta.data=...).

  • [ ] Note on "" should be addressed to the best of your abilities. E.g., median is not being imported (need to add @importFrom stats median) etc.

  • [ ] Some examples have extremely long runtimes (~30s vs. ~5s recommended); please try and reduced these.

HelenaLC avatar Aug 08 '24 10:08 HelenaLC

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

bioc-issue-bot avatar Aug 28 '24 12:08 bioc-issue-bot

Dear Helena

Thank you for giving our code a well-considered review. We have no question that this has helped to improve our package. In particular, we have made an effort to make this package more compatible with the SingleCellExperiment and SpatialExperiment classes. Before we get into specifics about this we'd like to address the elephant in the room.

We certainly understand the value of the inter-operability of Bioconductor classes and code. But at this point, Seurat is pretty much the standard for working with single cell data in R. The functionality that we have provided for working between Seurat on the one hand and SingleCellExperiment and SpatialExperiment on the other provides only a minimal extension to the off-the-shelf functions as.Seurat and as.SingleCellExperiment. What is needed here is a Bioconductor package devoted to inter-operability between these packages. This is a sizable undertaking, one well outside the scope of CatsCradle, and one which needs a dedicated team. But it would contribute significantly to a more sympatico relationship between these two worlds.

On to specifics:

All vignettes use a SeuratObject to demo capabilities. As pointed out previously, Seurat is not on Bioc. I appreciate the sentiment to assure usability across communities, however, please also include a vignette designated to using Bioc infrastructure, i.e., the SingleCellExperiment class alongside corresponding visualizations, e.g., using scater.

  • We have written a vignette devoted to the use of SingleCellExperiment. However, we have declined to use scater for plotting, choosing instead to use ggplot which we think is more widespread.

I noticed that predictAnnotation (possibly other functions) runs instantly for the SeuratObject , but stalls for the (identical) SingleCellExperiment . I traced this down to acceptor() , which calls on SCEtoSeurat , which is extremely inefficient. Granted this is exactly what is assuring interoperability with Bioc, it'd be much appreciated to assure that functions work equally on both types of objects (in terms of results and computation speed/cost).

  • We have significantly improved the running time of SCEtoSeurat.

In the DESCRIPTION, consider adding "Spatial" to biocViews: .

  • Done.

In the README, please add installation instructions using BiocManager::install() . We will be more than happy to do this when the package is approaching availability on Bioconductor. However, at the moment, we already have users in other labs accessing CatsCradle from github. Consider adding a NEWS file to keep track of changes, in non-technical language, in the code from one release version to the next (see here).

  • We've opened NEWS file, but so far, are a little bit short on news to put in it.

Seurat has been highly volatile over the years. When depending on it, I'd suggestion specifying exactly which version is expected to be compatible with your package (i.e., Seurat (>= x.y.z) ).

  • We are now requiring Seurat (>= 5.0.1)

Please consider implementing unit tests; we strongly encourage them!

  • In principle we like this idea. One drawback here is that we are constrained for space to provide example data which would make these tests meaningful. Fortunately, we find that the examples and vignettes act as de facto unit tests.

Please address build report NOTES to the best of you abilities; e.g., LazyData: in the DESCRIPTION should be set to false or removed, R version dependency should be >= 4.4.0, add URL: and BugReports: fields, add the output of the sessionInfo() at the vignette's conclusion, and more.

  • We are now requiring R >= 4.4.0
  • We now include BugReports: and URL fields in the DESCRIPTION file.
  • We have added sessionInfo to each of the vignettes.
  • We have set LazyData to FALSE. Note that this produces a warning which seems to result from roxygen not knowing where to find the data in question.

"S" is a rather unfortunate name for a package's data object (not unlikely that data(S) would overwrite the user's variable)... consider changing this to something more explicit, e.g., exSeuratObj or something.

  • This is now called exSeuratObj.

Data documentation is a little brief in some instances, e.g., humanLRN is missing source information or similar. Please see here for guidance, and include provenance and source information of all objects (accompanied by an inst/script/ , if applicable).

  • We now state explicitly humanLRN and mouseLRN are "taken from" rather than "derived from" nichenetr and give the url of the nichenetr paper.

I'd suggest consistently using code style for anything R-related (e.g., variable, function, class names) etc. -- if applicable, you can hyperlink external packages using BiocStyle functions, e.g., r BiocStyle::CRANpkg("ggplot2") and r BiocStyle::Biocpkg("scuttle") for packages hosted via CRAN and Bioc, respectively.

  • On the surface, this seems like a nice idea. But reading through our vignettes, we found it problematic. Is every reference to a Seurat object or a SingleCellExperiment object a reference to the Seurat package or the SingleCellExperiment package? The packages we refer to are quite well known, so we're unconvinced that the reader needs a link.

All functions are currently placed in 2 scripts with 1500 lines+ This makes things very hard to grasp, review, maintain. Consider placing functionally related code/helpers in separate scripts and, if applicable, using designated, say, utils-plotting.R, utils-spatial.R etc. scripts for code used repeatedly.

  • These have been broken out into 10 files with descriptive names and comments at the top of each file describing their contents.

In the build report, there are numerous WARNING related to namespace clashes (packages exporting identically named things). Most of these should be resolved by selective importing, which is in any case recommended; i.e., using @importFrom package function rather than @import to include a packages full namespace (see here).

  • We have replaced almost all @imports with @importFroms which resolves these conflicts.

Please also address the NOTE related to unused imports, namely, fossil,interp,knitr (the latter should go under Suggests: ).

  • Done.

NOTE on "partial argument match of 'meta' to 'meta.data'" CreateSeuratObject() should be resolved with CreateSeuratObject(..., meta.data=...) .

  • Done.

Note on "" should be addressed to the best of your abilities. E.g., median is not being imported (need to add @importFrom stats median ) etc.

  • We are now importing these functions, although they are directly available in base R.

Some examples have extremely long runtimes (~30s vs. ~5s recommended); please try and reduced these.

  • We've given this some thought and although we have a way to reduce these running times, we're reluctant to use it. Space limitations prevent us from providing additional example data. One way around this would be to downsize single cell data and matrices prior to using them in the examples, but this brings its own problems. For example, when randomly downsizing single cell (or, in this case "single gene") data we are likely to wind up losing entire clusters with unpredictable consequences. We think that these running times are simply the consequence of supplying and working with meaningful single cell data.

-----Original Message----- From: Bioconductor/Contributions @.> Sent: Aug 8, 2024 11:13 AM To: Bioconductor/Contributions @.> Cc: michaeldshapiro @.>, Mention @.> Subject: Re: [Bioconductor/Contributions] submitting new package CatsCradle (Issue #3452)

main * All vignettes use a SeuratObject to demo capabilities. As pointed out previously, Seurat is not on Bioc. I appreciate the sentiment to assure usability across communities, however, please also include a vignette designated to using Bioc infrastructure, i.e., the SingleCellExperiment class alongside corresponding visualizations, e.g., using scater.

  • I noticed that predictAnnotation (possibly other functions) runs instantly for the SeuratObject, but stalls for the (identical) SingleCellExperiment. I traced this down to acceptor(), which calls on SCEtoSeurat, which is extremely inefficient. Granted this is exactly what is assuring interoperability with Bioc, it'd be much appreciated to assure that functions work equally on both types of objects (in terms of results and computation speed/cost).

misc * In the DESCRIPTION, consider adding "Spatial" to biocViews:.

  • In the README, please add installation instructions using BiocManager::install().

  • Consider adding a NEWS file to keep track of changes, in non-technical language, in the code from one release version to the next (see here (https://contributions.bioconductor.org/news.html?q=news#news)).

  • Seurat has been highly volatile over the years. When depending on it, I'd suggestion specifying exactly which version is expected to be compatible with your package (i.e., Seurat (>= x.y.z)).

  • Please consider implementing unit tests (https://contributions.bioconductor.org/tests.html?q=unit#tests); we strongly encourage them!

  • Please address build report NOTES to the best of you abilities; e.g., LazyData: in the DESCRIPTION should be set to false or removed, R version dependency should be >= 4.4.0, add URL: and BugReports: fields, add the output of the sessionInfo() at the vignette's conclusion, and more.

  • "S" is a rather unfortunate name for a package's data object (not unlikely that data(S) would overwrite the user's variable)... consider changing this to something more explicit, e.g., exSeuratObj or something.

docs * Data documentation is a little brief in some instances, e.g., humanLRN is missing source information or similar. Please see here (https://contributions.bioconductor.org/docs.html#doc-inst-script) for guidance, and include provenance and source information of all objects (accompanied by an inst/script/, if applicable).

  • We encourage the use of r BiocStyle::Biocpkg("BiocStyle") for formatting (see here (https://contributions.bioconductor.org/docs.html?q=vignette#vignettes)).

  • I'd suggest consistently using code style for anything R-related (e.g., variable, function, class names) etc. -- if applicable, you can hyperlink external packages using BiocStyle functions, e.g., r BiocStyle::CRANpkg("ggplot2") and r BiocStyle::Biocpkg("scuttle") for packages hosted via CRAN and Bioc, respectively.

  • All package functions are currently placed in 2 scripts with 1500 lines+ This makes things very hard to grasp, review, maintain. Consider placing functionally related code/helpers in separate scripts and, if applicable, using designated, say, utils-plotting.R, utils-spatial.R etc. scripts for code used repeatedly.

code * In the build report, there are numerous WARNING related to namespace clashes (packages exporting identically named things). Most of these should be resolved by selective importing, which is in any case recommended; i.e., using @importFrom package function rather than @import to include a packages full namespace (see here (https://contributions.bioconductor.org/namespace.html?q=importfrom#imported-functions)).

  • Please also address the NOTE related to unused imports, namely, fossil,interp,knitr (the latter should go under Suggests:).

  • NOTE on "partial argument match of 'meta' to 'meta.data'" CreateSeuratObject() should be resolved with CreateSeuratObject(..., meta.data=...).

  • Note on "" should be addressed to the best of your abilities. E.g., median is not being imported (need to add @importFrom stats median) etc.

  • Some examples have extremely long runtimes (~30s vs. ~5s recommended); please try and reduced these.

— Reply to this email directly, view it on GitHub (https://github.com/Bioconductor/Contributions/issues/3452#issuecomment-2275455760), or unsubscribe (https://github.com/notifications/unsubscribe-auth/AL4ZXLBFEOB5YZDHAOAHUZLZQNADZAVCNFSM6AAAAABIYAYED6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDENZVGQ2TKNZWGA). You are receiving this because you were mentioned.Message ID: @.***>

michaeldshapiro avatar Aug 28 '24 12:08 michaeldshapiro

Dear Package contributor,

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

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

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.

The following are build products from R CMD build on the Single Package Builder: Linux (Ubuntu 22.04.3 LTS): CatsCradle_0.99.10.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 [email protected]:packages/CatsCradle 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 28 '24 13:08 bioc-issue-bot

Dear Michael, apologies in advance for the annoyance this might cause, but I hope you can understand our sentiment here from a 20+ years established community perspective...

In it's current shape, the package focuses on Seurat structures (as pointed out during pre-review already). -- reiterating core-team members here: -- With Seurat on CRAN, it does seem natural for your package to be on CRAN, too. Our ecosystem is striving for convenience to users and developers through direct reuse of established and essentially stable data structures like SingleCellExperiment. We are hitting resource limits for reviewing and QCing packages and so we would ask that if you want to have the package in Bioconductor, you enhance the direct interfaces to Bioc data structures directly (under-the-hood conversion and method dispatch to Seurat only is not sufficient); thanks!

Fwiw, SPOTlight is a good example of writing methods that work on basic data structures (like vectors/matrices), which are pulled out from SeuratObject/SingleCellExperiment objects, allowing for direct method dispatch to both.

HelenaLC avatar Aug 29 '24 04:08 HelenaLC