Bioconductor / Contributions

Contribute Packages to Bioconductor
135 stars 33 forks source link

SCIntRuler #3295

Closed yuelyu21 closed 2 months ago

yuelyu21 commented 9 months 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 9 months ago

Hi @yuelyu21

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: SCIntRuler
Title: Guiding the integration of multiple single-cell RNA-seq datasets
Version: 0.99.0
Authors@R: 
    person("Yue", "Lyu", , "yuelyu0521@gmail.com", role = c("aut", "cre"),
 comment = c(ORCID = "0000-0002-8912-6624"))
Description: The accumulation of single-cell RNA-seq (scRNA-seq) studies highlights the potential benefits of integrating multiple datasets. By augmenting sample sizes and enhancing analytical robustness, integration can lead to more insightful biological conclusions. However, challenges arise due to the inherent diversity and batch discrepancies within and across studies. SCIntRuler, a novel R package, addresses these challenges by guiding the integration of multiple scRNA-seq datasets.
License: MIT + file LICENSE
biocViews: Sequencing, GeneticVariability, SingleCell
Encoding: UTF-8
Roxygen: list(markdown = TRUE)
RoxygenNote: 7.3.0
Imports: 
    Rcpp,
    Matrix,
    batchelor,
    base,
    Seurat,
    MatrixGenerics,
    dplyr,
    coin,
    devtools,
    harmony,
    ggplot2,
    magrittr,
    stats,
    devtools
LinkingTo: Rcpp
URL: https://github.com/yuelyu21/SCIntRuler, https://yuelyu21.github.io/SCIntRuler/
BugReports: https://github.com/yuelyu21/SCIntRuler/issues
Suggests: 
    knitr,
    rmarkdown,
    testthat (>= 3.0.0)
VignetteBuilder: knitr
Depends: 
    R (>= 4.3.0)
LazyData: true
LazyDataCompression: xz
Config/testthat/edition: 3
lshep commented 8 months ago

A few initial comments

lshep commented 8 months ago

may we expect updates to address the above concerns?

bioc-issue-bot commented 7 months ago

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

Thank you for your interest in Bioconductor.

yuelyu21 commented 6 months ago

Hello Bioconductor Team,

I am writing to kindly request the reopening of my package submission issue. Over the past few weeks, I have addressed all the concerns raised during the initial review. Here are the specific changes and updates I've implemented:

  1. Code Improvements: I have refined the codebase for better performance and compliance with Bioconductor standards, ensuring that all functions are properly documented and efficient.
  2. Error Handling: Enhanced error handling across the package to provide clearer, more informative error messages to the users.
  3. Vignettes and Documentation: Updated and expanded the vignettes to provide more comprehensive usage examples and scenarios. Also, the documentation has been thoroughly revised for clarity and completeness.
  4. R CMD Check and BiocCheck: The package now passes R CMD check and BiocCheck with no errors, warnings.
  5. Active Maintenance Commitment: I am fully committed to the ongoing maintenance of the package, including timely updates for compatibility with new R and Bioconductor releases, and prompt responses to community feedback and issues.

I appreciate your consideration and am looking forward to continuing the submission process. Thank you for your time and support.

Best regards, Yue Lyu

lshep commented 5 months ago

You left some place holder sections that should be cleaned up like

\description{
A short description of your dataset.
}

Also when I try to R CMD build the package I get the following

Quitting from lines  at lines 286-295 [step 1] (SCIntRuler.Rmd)
Error: processing vignette 'SCIntRuler.Rmd' failed with diagnostics:
object 'onep_internal' not found
--- failed re-building ‘SCIntRuler.Rmd’

SUMMARY: processing the following file failed:
  ‘SCIntRuler.Rmd’

Error: Vignette re-building failed.
Execution halted
yuelyu21 commented 5 months ago

Hi, I have added some description for the dataset and also rebuilt the Vignette. I believe there should no errors now. Thanks!

lshep commented 5 months ago

There is still this:

man/sim_result.Rd:23:A short description of your dataset.

And please run R CMD build, R CMD check and R CMD BiocCheck. There are still ERROR when running checks.

yuelyu21 commented 5 months ago

Dear Bioconductor Core Team, thanks for pointting out the errors. Here, I first tried devtools::check(), it returns no error.

image

I then tried R CMD CHECK, status show "OK".

image

BiocCheck also gives no error no warning:

image

I believe now the package is ready to be checked again. Thanks for all your patience!

lshep commented 5 months ago

I am still getting an error in your tests when I test locally with R 4.40 Patched (2024-04-30 r86503) and Bioc 3.20. I'll move this forward but if the builds pick up this error as well it will need to be corrected before a reviewer is assigned to the package.

> sim_data <- SCEtoSeurat(sim_data_sce)
Error in cbind(seurat[["RNA"]]@meta.features, gene_metadata) :
  no slot of name "meta.features" for this object of class "Assay5"
Calls: SCEtoSeurat -> cbind
yuelyu21 commented 5 months ago

Dear Bioconductor Core Team, I fixed this issue and tries check_win_devel() function, which will build and check my package in a devel version of R on Win system. I got NO ERROR, NO WARNING. Here is the log.

New submission

Possibly misspelled words in DESCRIPTION: SCIntRuler (8:377) scRNA (8:55, 8:474)

bioc-issue-bot commented 5 months 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

yuelyu21 commented 5 months ago

Hey Bioconductor team, should I expect a build report to be posted here?

lshep commented 5 months ago

there should have I'll investigate why.

bioc-issue-bot commented 5 months 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.

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.

The following are build products from R CMD build on the Single Package Builder: Linux (Ubuntu 22.04.3 LTS): SCIntRuler_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/SCIntRuler to trigger a new build. A quick tutorial for setting up remotes and pushing to upstream can be found here.

lshep commented 5 months ago
cat 00install.out 
* installing *source* package ‘SCIntRuler’ ...
** using staged installation
** libs
using C++ compiler: ‘g++ (Ubuntu 11.4.0-1ubuntu1~22.04) 11.4.0’
g++ -std=gnu++17 -shared -L/home/biocbuild/bbs-3.20-bioc/R/lib -L/usr/local/lib -o SCIntRuler.so RcppExports.o crossdist.o -L/home/biocbuild/bbs-3.20-bioc/R/lib -lR
RcppExports.o: file not recognized: file format not recognized
collect2: error: ld returned 1 exit status
make: *** [/home/biocbuild/bbs-3.20-bioc/R/share/make/shlib.mk:10: SCIntRuler.so] Error 1
ERROR: compilation failed for package ‘SCIntRuler’
* removing ‘/home/pkgbuild/packagebuilder/workers/jobs/3295/R-libs/SCIntRuler’
yuelyu21 commented 5 months ago

Hi Bioconductor Team, I have deleted thoese files SCIntRuler.so RcppExports.o crossdist.o in my repo. It should work now. Thanks for all the support and patientce!

lshep commented 5 months ago

Please push to git.bioconductor.org to trigger a new build

bioc-issue-bot commented 5 months ago

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

bioc-issue-bot commented 5 months 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.

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.

The following are build products from R CMD build on the Single Package Builder: Linux (Ubuntu 22.04.3 LTS): SCIntRuler_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/SCIntRuler to trigger a new build. A quick tutorial for setting up remotes and pushing to upstream can be found here.

bioc-issue-bot commented 5 months ago

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

bioc-issue-bot commented 5 months 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 22.04.3 LTS): SCIntRuler_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/SCIntRuler to trigger a new build. A quick tutorial for setting up remotes and pushing to upstream can be found here.

bioc-issue-bot commented 5 months ago

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

yuelyu21 commented 5 months ago

Hi Marcel, please let me know if you have any questions, comments and suggestions!

yuelyu21 commented 4 months ago

@LiNk-NY , Could I kindly ask if it would be possible to expect some comments this week or next week?

LiNk-NY commented 4 months ago

Hi @yuelyu21 Apologies for the delay. I was away last week for a conference. I will provide a review today. Best, Marcel

LiNk-NY commented 4 months ago

Hi @yuelyu21 Thank you for your submission. Please see the review below.

Best regards, Marcel


SCIntRuler #3295

The package makes use of the Seurat representation and does not integrate with Bioconductor other than providing a serialized SummarizedExperiment dataset and conversion functions from SCE to Seurat. Please re-evaluate the need for a Bioconductor submission.

DESCRIPTION

NAMESPACE

vignette

bioc-issue-bot commented 4 months ago

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

bioc-issue-bot commented 4 months 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.

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.

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

bioc-issue-bot commented 4 months ago

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

bioc-issue-bot commented 4 months 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.

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.

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

bioc-issue-bot commented 4 months ago

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

bioc-issue-bot commented 4 months 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 22.04.3 LTS): SCIntRuler_0.99.5.tar.gz macOS 12.7.1 Monterey: SCIntRuler_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 git@git.bioconductor.org:packages/SCIntRuler to trigger a new build. A quick tutorial for setting up remotes and pushing to upstream can be found here.

yuelyu21 commented 4 months ago

Dear Marcel, @LiNk-NY

Thank you for your detailed and constructive feedback on the SCIntRuler package. I appreciate your time and effort in reviewing it and providing suggestions for improvement.

SCIntRuler was developed with the aim of providing a user-friendly tool for single-cell RNA-seq data integration, leveraging the popularity and functionality of Seurat. While I acknowledge that the current version primarily uses Seurat and includes conversion functions from SingleCellExperiment (SCE) to Seurat, I believe there is potential for this package to serve as a bridge between the Seurat and Bioconductor ecosystems.

I plan to include additional functionalities that operate directly on SummarizedExperiment and SingleCellExperiment objects, thus reducing dependency on Seurat. I am eager to continue developing the package to better align with Bioconductor’s standards and would greatly appreciate the opportunity to do so as part of the Bioconductor project.

Response to Specific Comments:

DESCRIPTION:

I removed the import of devtools as it is not essential for the package's functionality. NAMESPACE:

I removed the exportPattern function call. I considered using the native pipe |> rather than magrittr::'%>%' in the next update. Vignette:

I added a more descriptive VignetteIndexEntry. I used eval=FALSE for the Installation code chunk and uncomment the code. I removed the explicit installation of dependencies from the Installation code chunk, as they will be installed along with the package. I emoved the reference to devtools. I loaded the data with data("sim_data_sce", package = "SCIntRuler") to avoid using SCIntRuler::sim_data_sce. I ran a spell check on the vignette to ensure there are no spelling errors.

Thank you for considering my response. I look forward to your feedback and guidance on how I can further improve SCIntRuler to meet Bioconductor's expectations.

Best regards, Yue

yuelyu21 commented 4 months ago

@LiNk-NY ,Dear Marcel, Could I kindly ask if it would be possible to expect some comments this week?

LiNk-NY commented 4 months ago

Hi @yuelyu21

Thank you for following up and apologies for the delay.

The package is good to go as-is if you submit to CRAN, however, for the benefit of Bioconductor users, we would rather accept a package that makes use of Bioconductor data structures.

We thank you for your submission and patience. Feel free to open the issue once you have carefully considered the needs of the Bioconductor user base.

Best regards, Marcel

cc: @vjcitn

yuelyu21 commented 4 months ago

@LiNk-NY , Dear Marcel,

Thank you for your feedback on my package submission. I understand the importance of integrating Bioconductor data structures to better serve the Bioconductor user base.

My package is designed to use the Seurat package for its primary functionality and includes a function to convert SingleCellExperiment objects to Seurat objects. Given this dependency on Seurat, I have concerns about further modifying the package to align more closely with Bioconductor data structures without compromising its core functionality.

Could you please clarify if the current integration with Seurat and the conversion function are sufficient for consideration, or does this feedback mean that my package cannot be accepted by Bioconductor as it stands?

I appreciate your guidance and look forward to your response.

vjcitn commented 3 months ago

Dear @yuelyu21 -- we have been discussing your submission which is nicely done, I like the pkgdown site and the narration. I think it is important that you are interfacing to scanorama and harmony in this package. Yours seems to be the only one working with scanorama; a few others use harmony (see https://code.bioconductor.org/search/search?q=harmony). Because you have concerns about modifying the package to use Bioconductor data structures, and focus on Seurat structures, 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 like SingleCellExperiment. Thanks!

yuelyu21 commented 3 months ago

Dear Dr. Carey @vjcitn ,

Thank you very much for your feedback and kind words about my package submission. I appreciate the valuable comments and suggestions regarding interfacing with scanorama and harmony, and the importance of integrating Bioconductor data structures.

Given your advice and considering the natural fit of my package with Seurat on CRAN, I have decided to proceed with submitting my package to CRAN. I will certainly take your suggestions into account for future enhancements, especially regarding the integration with Bioconductor data structures.

Thank you again for your support and guidance.

Best, Yue Lyu