Bioconductor / Contributions

Contribute Packages to Bioconductor
135 stars 33 forks source link

MSstatsShiny #2783

Closed devonjkohler closed 2 years ago

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

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: MSstatsShiny
Type: Package
Title: Statistical Characterization of Post-translational Modifications
Version: 0.99.0
Date: 2022-09-26
Description: MSstatsShiny is an R-Shiny graphical user interface (GUI) 
  integrated with the R packages MSstats, MSstatsTMT, and MSstatsPTM. It 
  provides a point and click end-to-end analysis pipeline applicable to a wide 
  variety of experimental designs. These include data-dependedent acquisitions (DDA)
  which are label-free or tandem mass tag (TMT)-based, as well as DIA, SRM, and 
  PRM acquisitions and those targeting post-translational modifications (PTMs). 
  The application automatically saves users selections and builds an R script 
  that recreates their analysis, supporting reproducible data analysis.
Authors@R: c(
    person("Devon", "Kohler", email = "kohler.d@northeastern.edu", role = c("aut", "cre")),
    person("Olga", "Vitek", email = "o.vitek@northeastern.edu", role = "aut"))
License: Artistic-2.0
Depends: R (>= 4.2)
Imports: shiny, shinyBS, shinyjs, shinybusy, tidyverse, data.table, MSstats, 
  MSstatsTMT, MSstatsPTM, MSstatsConvert, knitr, readxl, gplots, marray, 
  ggrepel, plotly, Biobase, biomaRt, uuid, methods, utils, stats
Suggests: rmarkdown, tinytest, sessioninfo
VignetteBuilder: knitr
biocViews: ImmunoOncology, MassSpectrometry, Proteomics, Software, 
  DifferentialExpression, OneChannel, TwoChannel, Normalization, QualityControl
BugReports: https://github.com/Vitek-Lab/MSstatsPTM/issues
Encoding: UTF-8
Roxygen: list(markdown = TRUE)
RoxygenNote: 7.2.1
vjcitn commented 2 years ago

I think the "Title" field in DESCRIPTION may be wrong.

devonjkohler commented 2 years ago

Hi @vjcitn,

Thanks for pointing that out. I've updated the title on my end.

Devon

lshep commented 2 years ago

Please add ShinyApps to your biocViews fields.

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

devonjkohler commented 2 years ago

Hello,

Just curious where this package is in the review. Concerned that the deadline is approaching.

Devon

PeteHaitch commented 2 years ago

I've started the review but please appreciate that the reviewers' workload is substantially increased close to release. We aim to have reviews within 3 weeks of the package building successfully on the BioC build machine and I expect to provide yours in the next few days.

devonjkohler commented 2 years ago

Hi @PeteHaitch,

Thank you a bunch for taking a look at the package! I Definitely understand that the releases are a lot of work. I was just a little worried by the Wednesday deadline listed in the release schedule.

Devon

PeteHaitch commented 2 years ago

Hi @devonjkohler,

I've completed my initial review of MSstatsShiny. However, I have asked other Bioconductor package reviewers for comments because I do not have much experience with Shiny apps as a user or developer. In particular, I'm unsure how much the requirements and advice in the chapter on Shiny apps in the Developer & Reviewer's Guide have to be followed for acceptance of the package into Bioconductor.

I will say that my general impression is that the app is useful and running the example data through the app was easy to do and understand.

While we wait for the advice of the other reviewers and/or @Bioconductor/core, there are a number of Required points, as well as Recommended points, that I would ask you to first please address before acceptance into Bioconductor. Would you please provide line-by-line comments to my initial review so that I know what changes I'm looking for in my re-review.

Cheers, Pete

Required

Warning: replacing previous import ‘shinyjs::show’ by ‘marray::show’ when loading ‘MSstatsShiny’
Warning: replacing previous import ‘plotly::select’ by ‘biomaRt::select’ when loading ‘MSstatsShiny’

Recommended

devonjkohler commented 2 years ago

Hey @PeteHaitch,

Thank you for reviewing the package. Your comments make sense and I am in the process of addressing them now. I should have an updated version by tomorrow morning.

Best, Devon

PeteHaitch commented 2 years ago

Hi @devonjkohler,

I've received some feedback from other Bioconductor package reviewers about the guidelines in the chapter on Shiny apps in the Developer & Reviewer's Guide. From @vjcitn, Project Lead of Bioconductor, "At this point in our experience with (Shiny) apps, I'd propose that the rules in the guidelines be taken as advisory.", so we'll stick with that for this submission.

If you haven't already, please familiarise yourself with the guidelines and take them into consideration. The main guideline that I think you should seriously consider following is to have all the UI and server code under the R/ directory of the package (rather than under inst/MSshinyStats). This would then ensure that the UI and server code are checked by R CMD check and BiocCheck::BiocCheck().

Thanks, Pete

bioc-issue-bot commented 2 years ago

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

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

bioc-issue-bot commented 2 years ago

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

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.

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

devonjkohler commented 2 years ago

Hi @PeteHaitch,

I just pushed an updated version of the package responding to your review. I was able to address all your main points (see below).

In terms of the BioC guidelines for shiny packages I have reviewed them and they make sense. I agree that the code should be refactored under the R directory. In this update I was not able to change it because it is actually a very big undertaking, as the entire server side of the package needs to be refactored into functions. My hope is that we can use this version for the initial submission and refactor the code in subsequent releases. We are hoping to publish a paper on the application in the coming months and Bioconductor makes it much easier for users to install and use (especially because our user base is mainly people without a programming background).

I hope this makes sense!

Devon

Required

[x] Fix or otherwise address all WARNINGs raised by R CMD check and BiocCheck::BiocCheck()

[x] Related to the above, we recommend selective imports using @importFrom pkg fun roxygen2 tag rather than @import pkg. This will reduce the chances of name clashes, as in the warning above.

[x] Please use BiocStyle. for vignette formatting.

[x] Please add an Introduction section to the vignette. This should include motivation for inclusion to Bioconductor.

[x] Please add a table of contents to the vignette.

Recommended

It's generally a good idea to fix or otherwise address all NOTEs raised by R CMD check and BiocCheck::BiocCheck().

bioc-issue-bot commented 2 years ago

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.

PeteHaitch commented 2 years ago

Thank you for making the required changes and considering the recommended ones. I'm happy to accept MSstatsShiny into Bioconductor! Thank you for your contribution.

I think we've just snuck the package acceptance in before the before today's deadline but hopefully @lshep may be able confirm.

Some minor comments to your response.


In terms of the BioC guidelines for shiny packages I have reviewed them and they make sense. I agree that the code should be refactored under the R directory. In this update I was not able to change it because it is actually a very big undertaking, as the entire server side of the package needs to be refactored into functions. My hope is that we can use this version for the initial submission and refactor the code in subsequent releases. We are hoping to publish a paper on the application in the coming months and Bioconductor makes it much easier for users to install and use (especially because our user base is mainly people without a programming background).

That's totally fine :) As you can tell, our 'best-practises' for Shiny apps are still in-development and we aim to continue to develop them over the next release cycles. It will be valuable to hear from you as a Shiny developer about your experience (e.g., what you find valuable, points of friction, tips and tricks) on submitting a Shiny app to Bioconductor for incorporation into the guide.

I can imagine MSstatsShiny being very helpful to non-R users to analyse their proteomics data - the GUI is slick and convenient.

I went through the package, found all the imports, and replaced them with @importFrom (other than ggplot2 which calls a ton of different functions). There are no more clashes being returned by the package.

That's a fair trade off. If you're using lots of functionality from a dependency, then @import may be appropriate and simpler. @importFrom is preferable if you're only using a small subset of a dependency's functionality and to avoid explicit clashes.

I tried to address most of the notes here. The only remaining ones are generally related to code style. I am going to continue to address these as I refactor the shiny panels into functions.

The coding style stuff is generally only recommendations, so don't feel obliged to force your code into that style if it doesn't suit you or your colleagues style (e.g., using = instead of <- makes sense for you and your team).

PeteHaitch commented 2 years ago

Please double-check the syntax in your vignette, the headers don't seem quite right (see screenshot from opening the vignette on my computer)

image

Aside: did you mean to switch the vignette to a PDF rather than HTML? BiocStyle supports either (via BiocStyle::pdf_document or BiocStyle::html_document in the YAML frontmatter)

devonjkohler commented 2 years ago

Hi @PeteHaitch,

Thank you for pointing the vignette formatting issue out. I am adding in a fix now.

Devon

lshep commented 2 years ago

The master branch of your GitHub repository has been added to Bioconductor's git repository.

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/devonjkohler.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("MSstatsShiny"). The package 'landing page' will be created at

https://bioconductor.org/packages/MSstatsShiny

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.