Bioconductor / Contributions

Contribute Packages to Bioconductor
134 stars 33 forks source link

(inactive) CITEViz #2738

Closed gartician closed 1 year ago

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

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: CITEViz
Title: CITEViz
Version: 0.99.0
Authors@R: c(
    person('Garth', 'Kong', email = 'kongga2017@gmail.com', role = c('cre', 'aut'), comment = c(ORCID = '0000-0001-5262-5406')),
    person('Thai', 'Nguyen', email = 'tnguye14@uoregon.edu', role = 'aut', comment = c(ORCID = '0000-0002-8988-4003')),
    person('Wesley', 'Rosales', email = 'wesleykrosales@gmail.com', role = 'aut', comment = c(ORCID = '0000-0002-6381-9844')),
    person('Anjali', 'Panikar', email = 'anjali.panikar@gmail.com ', role = 'aut', comment = c(ORCID = '0000-0002-5740-807X')),
    person('John', 'Cheney', email = 'John.h.cheney@gmail.com', role = 'aut', comment = c(ORCID = '0000-0001-6194-2745')),
    person('Sarah', 'Carratt', email = 'carratt@ohsu.edu', role = 'rev', comment = c(ORCID = '0000-0003-2493-6789')),
    person('Brittany', 'Curtiss', email = 'curtisbr@ohsu.edu', role = 'rev', comment = c(ORCID = '0000-0002-5274-8323')),
    person('Theodore', 'Braun', email = 'braunt@ohsu.edu', role = 'rev', comment = c(ORCID = '0000-0002-6248-2143')),
    person('Julia', 'Maxson', email = 'maxsonj@ohsu.edu', role = 'rev', comment = c(ORCID = '0000-0002-3871-7221'))
    )
Description: CITEViz is a single-cell visualization platform with a custom module that replicates the interactive flow cytometry workflow in CITE-Seq. Users can can isolate cell populations of interest using the protein assay in CITE-Seq with minimal computational training. Additional features of CITEViz includes assessment of quality control metrics, feature visualization (feature expression and co-expression), and evaluation of clusters.
License: MIT + file LICENSE
Imports:
    bslib,
    config (>= 0.3.1),
    dplyr,
    DT,
    ggplot2,
    golem (>= 0.3.1),
    grDevices,
    magrittr,
    methods,
    plotly,
    rlang,
    SeuratObject,
    shiny (>= 1.6.0),
    stats,
    tibble,
    vembedr
Encoding: UTF-8
RoxygenNote: 7.2.0
Suggests:
    knitr,
    BiocStyle,
    rmarkdown,
    spelling,
    testthat (>= 3.0.0)
Config/testthat/edition: 3
Language: en-US
VignetteBuilder: knitr
biocViews: SingleCell, ShinyApps, FlowCytometry, QualityControl, GeneExpression
vjcitn commented 2 years ago

I am seeing that you have a tarball exceeding 5MB with a lot of large nice gifs under inst. We have to keep our package tarball sizes low. If you run BiocCheck::BiocCheck you will see that it produces an error on tarball size. I have to remove the pre-check passed notice.

gartician commented 2 years ago

Thank you Vince, we replaced all the large GIFs with still images (PNGs). Our BiocCheck run on our GitHub Actions looks dandy: https://github.com/maxsonBraunLab/CITEViz/runs/7717115756?check_suite_focus=true

gartician commented 2 years ago

I pushed my changes to GitHub, but I am confused about how to push my new changes to bioconductor's git repository. I tried to set my upstream using the command git remote add upstream git@git.bioconductor.org:packages/CITEViz.git, but it didn't work. I noticed I could not find CITEViz in this link , is that why I can't set an upstream because it doesn't exist?

vjcitn commented 2 years ago

Thanks for dealing with the images. @lshep this can be moderated. Garth it usually takes some time to get into git.

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

gartician commented 2 years ago

I'm going to pre-emptively address the warning that suggests me to evaluate more code chunks in the vignette.

One code chunk is used to show the readers how to install our program and run the app. Since this is a Shiny app, this makes the code chunk runs indefinitely which isn't ideal. One question though: should I instruct the users to install using the BioC method e.g. BiocInstall("CITEViz") as opposed to devtools?

Another code chunk shows how to do differential expression in gated cells, but that requires importing a 120MB test dataset. I didn't include that dataset here because that's not part of the app, but I did add links for the user to get these external files.

PeteHaitch commented 2 years ago

Thanks for your submission of CITEViz, @gartician. I'll be reviewing the package but won't be able to start the formal review until next week.

should I instruct the users to install using the BioC method e.g. BiocInstall("CITEViz") as opposed to devtools?

It is recommended to only refer to (or otherwise emphasise) the 'official' Bioconductor installation instructions, to avoid issues with people installing incompatible versions of software when using the 'GitHub' installation instructions.

I didn't include that dataset here because that's not part of the app, but I did add links for the user to get these external files.

I've not yet looked closely at the package, so I'm not sure which test dataset you are referring to, but please consider making the data available via AnnotationHub or ExperimentHub (https://contributions.bioconductor.org/data.html).

bioc-issue-bot commented 2 years ago

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

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

gartician commented 2 years ago

Hi @PeteHaitch, I hope you were able to take a look at this package during the week.

That last update was to replace devtools installation instructions with BiocManager::install("CITEViz").

How important is it to create an ExperimentHub/AnnotationHub for this dataset? The test dataset I mentioned is just a subset of of the data from this paper paper and this Seurat vignette. Since it's readily available for download as an RDS file, I wonder if it's more work than necessary to include into this package. If I am mistaken and there's good reason to include one, I will be glad to learn more about it.

PeteHaitch commented 2 years ago

@Bioconductor/core: could you please respond to @gartician's query:

How important is it to create an ExperimentHub/AnnotationHub for this dataset? The test dataset I mentioned is just a subset of of the data from this paper paper and this Seurat vignette. Since it's readily available for download as an RDS file, I wonder if it's more work than necessary to include into this package. If I am mistaken and there's good reason to include one, I will be glad to learn more about it.

lshep commented 2 years ago

Without looking closely at the package... You need to be able to run or test run the code of the package. If there are smaller datasets that are executed in tests and man page code than perhaps that is okay... but it depends on what is exported and how functions are run in the package. The other alternative too would be instead of using ExperimentHub, if the data is readily downloadable, you could download and use BiocFileCache to at least have runnable examples.

PeteHaitch commented 2 years ago

Hi @gartician,

I started to review CITEViz, but it needs substantial changes before it can be considered for Bioconductor.

One of the central aims of Bioconductor is to provide interoperable software, but CITEViz does not support SingleCellExperiment objects, the core Bioconductor data structure for storing single-cell data. Support for SingleCellExperiment objects is a requirement for inclusion into Bioconductor.

I don't mean to discourage the submission of CITEViz to Bioconductor but I want to be realistic about expectations and requirements (covered in detail in https://contributions.bioconductor.org/). If supporting SingleCellExperiment objects is beyond the current scope of the package, then CRAN might be a suitable repository for CITEViz since it has no such interoperability requirements.

If you intend to support SingleCellExperiment objects and would to proceed with the Bioconductor submission, please let us know. I do have some additional comments from my initial look but we won't proceed further with the review until this is resolved.

Cheers, Pete

gartician commented 2 years ago

Thank you @PeteHaitch for the initial review, I'll have a chat with my team to include support for SCE objects to provide interoperable software in BioConductor.

gartician commented 2 years ago

Hi @PeteHaitch, we are working to accept SCE objects into CITEViz. We plan to use a file format flag that determines how the data will be queried based on the input file. This should help us build things more hierarchically with a reasonable coding structure. Thank you for the feedback so far!

gartician commented 2 years ago

Hi @PeteHaitch, we were able to include SCE objects to CITEViz! For some new functions we added, should we include the tests into the next commit? We will definitely finish writing tests before the BioConductor 3.16 release, but one of our developers is out sick and may require an extra week or two to finish. Meanwhile, we are available to address additional comments if possible. Thank you for your input, let us know what you think.

PeteHaitch commented 2 years ago

Hi @gartician,

That's great news!

According to the Bioconductor 3.16 Release Schedule, we have until October 26 for the package to be accepted in order for it to make the 3.16 release. I'm confident we can achieve this and will work with you to help as best I can.

I'll do a complete review once the new version of the package is submitted and passing the automatic checks. In the meantime, here are some notes from my initial look at the package that will need to be addressed:

vjcitn commented 2 years ago

I am looking at 0.99.4. The comments from Pete above seem to hold ... can't find example data readily when starting the app. I'd also add a few points

I hope these can be addressed along with Pete's comments in the near future.

gartician commented 2 years ago

Thank you for the additional comments! We have more documentation coming up soon, which should address most of the comments. I'll definitely rephrase the custom module sentence as it can be misinterpreted. I just wanted to see if I am pushing to the right places, and the changes are being tracked correctly. Thanks!

gartician commented 1 year ago

I do have a general question about ExperimentHub and data sharing. I was wondering if it's ethically okay for us to publish another EH package if we didn't generate the data ourselves? All I did was download the original RDS and down-sampled the data, which I don't feel warrants my name on it.

As an alternative, can we provide a link to a Google Drive similar to the vignette found in excluderanges? I did include a link to a Google Drive folder for the test data PBMC (120MB) in the original CITEViz preprint in the Supplementary Information section. Now that I am aware it is hard to find, what I can do is advertise it better.

As this is my first time submitting a package, I am still understanding how things are done efficiently so please excuse the newbie questions. Thank you in advanced!

vjcitn commented 1 year ago

@lshep will have the best advice here. As long as you make proper attributions I think there is no problem with you using EH for data that is integral to your package. As for Google Drive, I am not familiar with that approach, but if the resource is durable and open, it sounds OK. zenodo is probably a better option in terms of durability and cost.

lshep commented 1 year ago

We in generally don't allow or recommend using a google drive to distribute data and have asked maintainers to do this to move the data to a more permanent secure data distribution server. The excluderanges uses data from the hubs and not google drive in the package itself which I think why this slipped through the cracks.
As far as okay to publish the data. I agree with @vjcitn that you should give the original data credit and make sure you check the licensing of the data that you are down-sampling to make sure it allow redistribution.

gartician commented 1 year ago

AdditionalPackage: https://github.com/maxsonBraunLab/CITEVizTestData

bioc-issue-bot commented 1 year ago

Can't build unless issue is open and '2. review in progress' label is present, or issue is closed and 'TESTING' label is present.

gartician commented 1 year ago

Can anyone adjust the labels to start building the test data?

lshep commented 1 year ago

Please try again

gartician commented 1 year ago

AdditionalPackage: https://github.com/maxsonBraunLab/CITEVizTestData

bioc-issue-bot commented 1 year ago

Hi @gartician, Thanks for submitting your additional package: https://github.com/maxsonBraunLab/CITEVizTestData. We are taking a quick look at it and you will hear back from us soon.

bioc-issue-bot commented 1 year ago

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

bioc-issue-bot commented 1 year 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/CITEViz to trigger a new build. A quick tutorial for setting up remotes and pushing to upstream can be found here.

gartician commented 1 year ago

Hi @PeteHaitch and @vjcitn,

The last update should address your concerns about including SCE objects, using ExperimentHub to host a test dataset, and fix illegible figures in the vignette. Furthermore, we refactored the code with better helper functions to retrieve data from Seurat/SCE objects to improve readability. Thank you for taking another look!

PeteHaitch commented 1 year ago

Thanks @gartician Please also address the warnings identified by the package builder; the build report is included in https://github.com/Bioconductor/Contributions/issues/2738#issuecomment-1283492916 I'll begin my review once that's cleared up

bioc-issue-bot commented 1 year ago

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

bioc-issue-bot commented 1 year 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/CITEViz to trigger a new build. A quick tutorial for setting up remotes and pushing to upstream can be found here.

bioc-issue-bot commented 1 year ago

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

bioc-issue-bot commented 1 year 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/CITEViz to trigger a new build. A quick tutorial for setting up remotes and pushing to upstream can be found here.

bioc-issue-bot commented 1 year ago

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

bioc-issue-bot commented 1 year 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/CITEViz to trigger a new build. A quick tutorial for setting up remotes and pushing to upstream can be found here.

gartician commented 1 year ago

OK this should be good to review, and we look forward to your response. The last push was just a minor change in the vignette, nothing major.

PeteHaitch commented 1 year ago

I'll return to reviewing packages now that the release of Bioconductor 3.16 is done. However, I also have a Masters thesis to review and abstracts for BioC Asia to review, both with deadlines next week, so I'm afraid I won't be able to start my package reviews until those commitments are behind me. Thanks for your patience.

bioc-issue-bot commented 1 year ago

Additional Package has been approved for building.

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.

bioc-issue-bot commented 1 year 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/CITEVizTestData to trigger a new build. A quick tutorial for setting up remotes and pushing to upstream can be found here.

bioc-issue-bot commented 1 year ago

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

bioc-issue-bot commented 1 year 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/CITEVizTestData to trigger a new build. A quick tutorial for setting up remotes and pushing to upstream can be found here.

bioc-issue-bot commented 1 year ago

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

bioc-issue-bot commented 1 year 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/CITEVizTestData to trigger a new build. A quick tutorial for setting up remotes and pushing to upstream can be found here.

bioc-issue-bot commented 1 year ago

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

bioc-issue-bot commented 1 year 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/CITEVizTestData to trigger a new build. A quick tutorial for setting up remotes and pushing to upstream can be found here.