Contributions icon indicating copy to clipboard operation
Contributions copied to clipboard

dominoSignal

Open jmitchell81 opened this issue 1 year ago • 9 comments

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

  • Repository: https://github.com/FertigLab/dominoSignal

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.

jmitchell81 avatar May 13 '24 14:05 jmitchell81

Hi @jmitchell81

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: dominoSignal
Title: Cell Communication Analysis for Single Cell RNA Sequencing
Version: 0.99.2
Authors@R: c(
    person("Christopher", "Cherry", role = c("aut"), email = "[email protected]", comment = c(ORCID = "0000-0002-5481-0055")),
    person("Jacob T", "Mitchell", role = c("aut", "cre"), email = "[email protected]", comment = c(ORCID = "0000-0002-5370-9692")),
    person("Sushma", "Nagaraj", role = c("aut"), email = "[email protected]", comment = c(ORCID = "0000-0001-5166-1309")),
    person("Kavita", "Krishnan", role = c("aut"), email = "[email protected]", comment = c(ORCID = "0000-0003-1345-0249")),
    person("Dmitrijs", "Lvovs", role = c("aut"), email = "[email protected]"),
    person("Elana", "Fertig", role = "ctb", email = "[email protected]", comment = c(ORCID = "0000-0003-3204-342X")),
    person("Jennifer", "Elisseeff", role = "ctb", email = "[email protected]", comment = c(ORCID = "0000-0002-5066-1996"))
    )
Description: 
    dominoSignal is a package developed to analyze cell signaling through ligand - receptor - transcription factor networks in scRNAseq data. It takes as input information transcriptomic data, requiring counts, z-scored counts, and cluster labels, as well as information on transcription factor activation (such as from SCENIC) and a database of ligand and receptor pairings (such as from cellphoneDB). This package creates an object storing ligand - receptor - transcription factor linkages by cluster and provides several methods for exploring, summarizing, and visualizing the analysis.
BugReports: https://github.com/FertigLab/dominoSignal/issues
Depends:
    R(>= 4.2.0),
Imports:
    biomaRt,
    ComplexHeatmap,
    circlize,
    ggpubr,
    grDevices,
    grid,
    igraph,
    Matrix,
    methods,
    plyr,
    stats,
    utils
License: GPL-3 | file LICENSE
Encoding: UTF-8
LazyData: false
RoxygenNote: 7.3.1
biocViews:
    SystemsBiology,
    SingleCell,
    Transcriptomics,
    Network
Suggests:
    knitr,
    patchwork,
    rmarkdown,
    Seurat,
    testthat,
    formatR,
    BiocFileCache,
    SingleCellExperiment
VignetteBuilder: knitr
Language: en-US
Roxygen: list(markdown = TRUE)
URL: https://fertiglab.github.io/dominoSignal/

bioc-issue-bot avatar May 13 '24 14:05 bioc-issue-bot

Please see previous comments on initial submission. In summary:

vignette

  • [ ] Please show things like options and set.seeds to users so they can accurately run and reproduce the code in the vignette.

  • [ ] We don't allow data to be on github. can the data retrieved from github also be moved to a trusted site like zenodo.

  • [ ] Vignette code must be executable. Currently no code is evaluated.

General

  • [ ] There is currently no interop with Bioconductor standard classes for single cell. Please adjust package to also account for this and show examples. See SingleCellExperiment packages/class.

  • [ ] Packages used in vignettes and examples must be on CRAN or Bioconductor.

It also appears the re-naming is not complete:

R CMD build dominoSignal 
* checking for file 'dominoSignal/DESCRIPTION' ... OK
* preparing 'dominoSignal':
* checking DESCRIPTION meta-information ... OK
* installing the package to build vignettes
* creating vignettes ... ERROR
--- re-building ‘domino2.Rmd’ using rmarkdown

Quitting from lines  at lines 24-32 [libraries] (domino2.Rmd)
Error: processing vignette 'domino2.Rmd' failed with diagnostics:
there is no package called 'domino2'
--- failed re-building ‘domino2.Rmd’

lshep avatar May 20 '24 13:05 lshep

Hello @lshep. My apologies for this issues that were carried over from the last submission. We had an issue with our GitHub action for pushing development changes to this submission branch. I will have these resolved quickly.

To your point about data from GitHub, is this in regards points in the vignettes when data from the cellPhoneDB ligand-receptor database is downloaded from https://github.com/ventolab/cellphonedb-data/archive/refs/tags/v4.0.0.tar.gz ? This is data curated by another laboratory that we do not have permission to re-host on zenodo. We could use a toy data base for vignette examples if need be while directing to this data base resource in the documentation if we are not allowed to directly download from this source in the scripting of the vignette.

jmitchell81 avatar May 20 '24 14:05 jmitchell81

you can leave the github data for now and we can look into further in the review.

lshep avatar May 20 '24 15:05 lshep

Thank you @lshep. I have corrected our submission branch at HEAD. We're ready for your review.

jmitchell81 avatar May 20 '24 19:05 jmitchell81

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 May 29 '24 16:05 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.

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

bioc-issue-bot avatar May 31 '24 16:05 bioc-issue-bot

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 Jun 03 '24 16:06 bioc-issue-bot

Hi @jmitchell81,

Here is some immediate feedback:

  1. I still don't see any set.seed() calls in the HTML vignettes.

  2. You still have at least one occurence of domino2 in your doc:

    image

    Please make sure to find and correct them all.

  3. It doesn't seem that you use standard markdown syntax in your .Rmd files. As a consequence there are many small cosmetic problems with the rendering of the HTML vignettes. See for example \underline{named} vector or {SingleCellExperiment} here:

    image

  4. Four mysterious lines of code right below the title of the vignette and before any text:

    image

    That's quite an unusual and pretty dry way to start a vignette! What is this code? What does it do? Why is it here? Does it work properly without explicitly loading any package first? What package(s)?

  5. For example, if I run these four lines of code in a fresh R session and try to display the dom object, I get hundreds of screens of output. This is unfortunate because domino objects actually have a show() method whose purpose is to prevent that. But that method is defined in the dominoSignal package and since the package is not loaded the method is not found. Or maybe it's because the package does not export the method, I'm not sure. In any case you need to export that method.

  6. Many of the examples in the man pages use a non-exported non-documented symbol (dominoSignal:::pbmc_dom_built_tiny). This is not good practice. You should only expose symbols that are exported and documented to the user.

Please address before I take a closer look at the package.

Thanks, H.

hpages avatar Jun 19 '24 01:06 hpages

Hi @jmitchell81,

Do you intend to follow up on this submission? Let me know if you have any questions.

Best, H.

hpages avatar Jul 18 '24 22:07 hpages

Thank you for checking in @hpages. Coincidentally, our updates addressing your feedback were just finished today. The HEAD branch of our package is ready for your review.

Best regards, Jacob

jmitchell81 avatar Jul 18 '24 22:07 jmitchell81

plz bump version and push so single package builder processes

vjcitn avatar Jul 18 '24 23:07 vjcitn

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

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

Thank you @vjcitn. I've pushed to the git.bioconductor.org repo. The version number has been bumped to 0.99.3 . The commit id should be f660117. The full id posted by the bio-issue-bot above appears to be correct.

jmitchell81 avatar Jul 19 '24 15:07 jmitchell81

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: macOS 12.7.1 Monterey: dominoSignal_0.99.3.tar.gz Linux (Ubuntu 22.04.3 LTS): dominoSignal_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 [email protected]:packages/dominoSignal 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 19 '24 15:07 bioc-issue-bot

@jmitchell81 Thanks for these changes.

Two more things and the package should be good to go:

  1. You still have many occurences of "domino2" in your source code. Note that there are plenty of tools that let you find all occurences of a given word in your source code (e.g. most GUI provide such a tool). Please take the time to familiarize yourself with these tools. Finding all occurences of a given word in your code is such a common task that it's an essential developer skill.

  2. Installation section in the "Get Started with dominoSignal" vignette: Never install a Bioconductor package from Github as this will typically mix-up packages from different Bioconductor versions, leading up to obscure errors, incorrect results, and a lot of confusion and frustration for all parties involved. Please show the proper/safe way, which is to use BiocManager::install(). See for example https://bioconductor.org/packages/SummarizedExperiment

Thanks again,

H.

hpages avatar Jul 30 '24 18:07 hpages

Thank you for your feedback @hpages .

For comment 7 on occurrences of "domino2" in our source code, I found 6 occurrences in comments within our tests and one occurrence in the NEWS.md signifying where we changed the package name. Were there other occurrences you found that we need to address?

jmitchell81 avatar Jul 30 '24 19:07 jmitchell81

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

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

Hello @hpages. We proceeded with addressing your comments to remove mentions of the old "domino2" package name and to update the installation instructions to use biocManager. We have, however, left in one instance of "domino2" in NEWS.md that we'd like to keep to signify the version at which the package name was changed to "dominoSignal". We await your further review and appreciate your feedback.

jmitchell81 avatar Aug 05 '24 18:08 jmitchell81

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: macOS 12.7.1 Monterey: dominoSignal_0.99.4.tar.gz Linux (Ubuntu 22.04.3 LTS): dominoSignal_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/dominoSignal 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 05 '24 18:08 bioc-issue-bot

Thanks for the changes.

Best, H.

hpages avatar Sep 02 '24 16:09 hpages

Your package has been accepted. It will be added to the Bioconductor nightly builds.

Thank you for contributing to Bioconductor!

Reviewers for Bioconductor packages are volunteers from the Bioconductor community. If you are interested in becoming a Bioconductor package reviewer, please see Reviewers Expectations.

bioc-issue-bot avatar Sep 02 '24 16:09 bioc-issue-bot

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

To use the git.bioconductor.org repository, we need an 'ssh' key to associate with your github user name. If your GitHub account already has ssh public keys (https://github.com/jmitchell81.keys is not empty), then no further steps are required. Otherwise, do the following:

  1. Add an SSH key to your github account
  2. Submit your SSH key to Bioconductor

See further instructions at

https://bioconductor.org/developers/how-to/git/

for working with this repository. See especially

https://bioconductor.org/developers/how-to/git/new-package-workflow/ https://bioconductor.org/developers/how-to/git/sync-existing-repositories/

to keep your GitHub and Bioconductor repositories in sync.

Your package will be included in the next nigthly 'devel' build (check-out from git at about 6 pm Eastern; build completion around 2pm Eastern the next day) at

https://bioconductor.org/checkResults/

(Builds sometimes fail, so ensure that the date stamps on the main landing page are consistent with the addition of your package). Once the package builds successfully, you package will be available for download in the 'Devel' version of Bioconductor using BiocManager::install("dominoSignal"). The package 'landing page' will be created at

https://bioconductor.org/packages/dominoSignal

If you have any questions, please contact the bioc-devel mailing list (https://stat.ethz.ch/mailman/listinfo/bioc-devel); this issue will not be monitored further.

lshep avatar Sep 03 '24 15:09 lshep