Bioconductor / Contributions

Contribute Packages to Bioconductor
131 stars 33 forks source link

domino2 #3259

Closed jmitchell81 closed 1 month ago

jmitchell81 commented 6 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 6 months ago

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: domino2
Title: Cell Communication Analysis for Single Cell RNA Sequencing
Version: 0.99.1
Authors@R: c(
    person("Christopher", "Cherry", role = c("aut"), email = "ccherry.6574@gmail.com", comment = c(ORCID = "0000-0002-5481-0055")),
    person("Jacob T", "Mitchell", role = c("aut", "cre"), email = "jmitch81@jhmi.edu", comment = c(ORCID = "0000-0002-5370-9692")),
    person("Sushma", "Nagaraj", role = c("aut"), email = "snagara5@jhmi.edu", comment = c(ORCID = "0000-0001-5166-1309")),
    person("Kavita", "Krishnan", role = c("aut"), email = "kkrishnan@jhmi.edu", comment = c(ORCID = "0000-0003-1345-0249")),
    person("Dmitrijs", "Lvovs", role = c("aut"), email = "dlvovs1@jhmi.edu"),
    person("Elana", "Fertig", role = "ctb", email = "ejfertig@jhmi.edu", comment = c(ORCID = "0000-0003-3204-342X")),
    person("Jennifer", "Elisseeff", role = "ctb", email = "jhe@jhu.edu", comment = c(ORCID = "0000-0002-5066-1996"))
    )
Description: 
    Domino2 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/domino2/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.2.3
biocViews:
    SystemsBiology,
    SingleCell,
    Transcriptomics,
    Network
Suggests:
    knitr,
    patchwork,
    rmarkdown,
    Seurat,
    testthat,
    formatR
VignetteBuilder: knitr
Language: en-US
Roxygen: list(markdown = TRUE)
lshep commented 6 months ago

I'm slightly concerned with the name. The currently active domino package on CRAN seems to have completely different functionality?

We also like to have a clean branch to add into the Bioconductor git repository. There are additional files and directories like pkgdown, old.README, scenic_bash, .github/workflows. We understand these tools can be useful and recommend moving them to a different branch on github. Can the directory structure be cleaned up?

You seem to download a lot of data in the vignette. Is this all example data? it will likely be worth while to cache the data files being download especially since the Bioconductor builders have build and check time constraints. See BiocFileCache

All vignettes should be rendered through the R cmd build of a package and products should not be included in the docs directory.

Currently when I try to do R CMD build locally it fails

orikern@jbcj433:~/PkgReview$ R CMD build domino2 
* checking for file 'domino2/DESCRIPTION' ... OK
* preparing 'domino2':
* checking DESCRIPTION meta-information ... OK
* installing the package to build vignettes
* creating vignettes ... ERROR
--- re-building ‘domino2.Rmd’ using rmarkdown
trying URL 'https://zenodo.org/records/10222767/files/pbmc_seurat.rds'
Content type 'application/octet-stream' length 294035980 bytes (280.4 MB)
===========================================
downloaded 241.4 MB

Quitting from lines 35-80 [files] (domino2.Rmd)
Error: processing vignette 'domino2.Rmd' failed with diagnostics:
download from 'https://zenodo.org/records/10222767/files/pbmc_seurat.rds' failed
--- failed re-building ‘domino2.Rmd’

--- re-building ‘domino_object_vignette.Rmd’ using rmarkdown
trying URL 'https://zenodo.org/records/10222767/files/pbmc_domino_built.rds'
Content type 'application/octet-stream' length 287884913 bytes (274.5 MB)
==================================================
downloaded 274.5 MB

--- finished re-building ‘domino_object_vignette.Rmd’

--- re-building ‘plotting_vignette.Rmd’ using rmarkdown
trying URL 'https://zenodo.org/records/10222767/files/pbmc_domino_built.rds'
Content type 'application/octet-stream' length 287884913 bytes (274.5 MB)
==================================================
downloaded 274.5 MB

Warning in doTryCatch(return(expr), name, parentenv, handler) :
  semi-transparency is not supported on this device: reported only once per page
Warning in doTryCatch(return(expr), name, parentenv, handler) :
  semi-transparency is not supported on this device: reported only once per page
--- finished re-building ‘plotting_vignette.Rmd’

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

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

@jmitchell81 do you have comments or updates to the above?

jmitchell81 commented 5 months ago

Thank you @lshep, we have been working to address your comments. In regards to the naming of the package, we have suggested alternative names to the investigators that originally wrote and published this package to ensure continuity with the published name of "domino" that has been used in subsequent publications while distinguishing from the package on CRAN and the "DominoEffect" package on Bioconductor.

We have been able to implement the BiocFileCache suggestion you provided for our vignettes to ensure the data for the vignette can be accessed without timing out. This timeout is likely the reason you were unable to render the vignette as we have been able to successfully build the package with GitHub actions in it's current repository in the FertigLab organization.

For the additional docs in the package, we are working to maintain documentation and interactive vignettes with pkgdown on a separate branch and maintain a dedicated clean branch where pushes to Bioconductor could be made. For this submission of our package, can we specify a branch of our GitHub repository for the submission rather that the HEAD branch of the repository?

Thank you for your help.

lshep commented 4 months ago

Please keep us informed about the naming. Its difficult to rename a package after it is accepted to Bioconductor.
I will check the updates on BiocFileCache and try to rebuild/recheck later today.
As far as the branches, our automated scripts that we use to place a package on our system uses the defined "default" branch of the github. If possible to change the clean branch as default for a day or two when we are agreed to move the package forward in the review process; after that point you can change back to whatever default branch you would like on github as Bioconductor will only build off code pushed to git.bioconductor.org. Hopefully this would be agreeable?

jmitchell81 commented 4 months ago

Thank you @lshep. We have prepared a branch for the package called "bioconductor" that includes the caching updates and removed the additional documentation from pkgdown. The name is still set as "domino2" for the moment as I am still debugging a unit test that compared results to results from an older version of the package. We still intend upon updating the package's name, but would appreciate your review of the changes made to ensure the package will build for you. The default branch will be set to "bioconductor" for your review for the rest of today.

lshep commented 4 months ago

General

That being said, I did glance at the package and have the following additional comments:

vignette

General

jmitchell81 commented 4 months ago

Thank you for your feedback @lshep. I have a question for clarification about the data on GitHub. In our current vignettes and in some unit tests, we do retrieve data from Zenodo that is evaluated and processed through this vignette. The code that is evaluated in the R markdown file for our vignette does not use any data hosted on GitHub. We do include code that is not evaluated as part of the vignette on how to obtain ligand-receptor reference data from CellPhoneDB from the data's original host location on GitHub. Even though this code is not evaluated, must it be removed from our vignette to meet Bioconductor standards?

Among our development group for the domino package, we deliberated between re-hosting all data used in our vignettes on Zenodo or linking to original sources of the reference annotation of ligand-receptor pairs and openly available scRNAseq data in order to credit those who have made those resources public. We have proceeded with the later option, but can adapt to only using data hosted on Zenodo if need be.

jmitchell81 commented 1 month ago

@lshep Thank you for reviewing our submission. We have updated the package's name to dominoSignal and addressed the tasks you had set out in your prior review. The current HEAD branch at https://github.com/FertigLab/dominoSignal contains the current state of the package we would like to submit.

With the change of the package's name, can we continue the issue thread of this submission, or should I begin a new submission under the new name?

lshep commented 1 month ago

Please open a new issue. It will be easier to ensure all of the configuration is set up correctly. Thank you.