Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

gg4way #3129

Closed
10 tasks done
ben-laufer opened this issue Aug 30, 2023 · 15 comments
Closed
10 tasks done

gg4way #3129

ben-laufer opened this issue Aug 30, 2023 · 15 comments
Assignees
Labels
3a. accepted will be ingested into Bioconductor daily builder for distribution OK

Comments

@ben-laufer
Copy link

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 understand that by submitting my package to Bioconductor,
    the package source and all review commentary are visible to the
    general public.

  • I have read the Bioconductor Package Submission
    instructions. My package is consistent with the Bioconductor
    Package Guidelines.

  • I understand Bioconductor Package Naming Policy and acknowledge
    Bioconductor may retain use of package name.

  • 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.

  • My package addresses statistical or bioinformatic issues related
    to the analysis and comprehension of high throughput genomic data.

  • 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.

  • 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:

  • The 'devel' branch for new packages and features.
  • The stable 'release' branch, made available every six
    months, for bug fixes.
  • 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.

@bioc-issue-bot
Copy link
Collaborator

Hi @ben-laufer

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: gg4way
Title: 4way Plots of Differential Expression
Version: 0.99.0
Authors@R: c(
    person(given = "Ben",
 family = "Laufer",
 role = c("aut", "cre"),
 email = "blaufer@gmail.com",
 comment = c(ORCID = "0000-0002-7558-1335")),
    person(given = "Brad",
 family = "Friedman",
 role = c("aut")))
Description: 4way plots enable a comparison of two contrasts of differential gene expression. The gg4way package creates 4way plots using the ggplot2 framework and supports popular Bioconductor objects. The package also provides information about the correlation between contrasts and significant genes of interest.
License: MIT + file LICENSE
URL: https://github.com/ben-laufer/gg4way
BugReports: https://github.com/ben-laufer/gg4way/issues
biocViews: 
    Software,
    Visualization,
    DifferentialExpression,
    GeneExpression,
    Transcription,
    RNASeq,
    SingleCell,
    Sequencing
Encoding: UTF-8
Roxygen: list(markdown = TRUE)
RoxygenNote: 7.2.3
Depends:
    R (>= 4.3.0),
    ggplot2
Imports: 
    DESeq2,
    dplyr,
    ggrepel,
    glue,
    janitor,
    limma,
    magrittr,
    methods,
    purrr,
    rlang,
    scales,
    stats,
    stringr,
    tibble,
    tidyr
Suggests: 
    airway,
    BiocStyle,
    edgeR,
    knitr,
    org.Hs.eg.db,
    rmarkdown,
    testthat
VignetteBuilder: knitr
Config/testthat/edition: 3
LazyData: false

@bioc-issue-bot bioc-issue-bot added the 1. awaiting moderation submitted and waiting clearance to access resources label Aug 30, 2023
@vjcitn vjcitn added the pre-check passed pre-review performed and ready to be added to git label Sep 12, 2023
@bioc-issue-bot
Copy link
Collaborator

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 bioc-issue-bot added pre-review on bioconductor git and access to on demand build but not assigned reviewer until build report clean and removed 1. awaiting moderation submitted and waiting clearance to access resources pre-check passed pre-review performed and ready to be added to git labels Sep 14, 2023
@bioc-issue-bot
Copy link
Collaborator

Dear Package contributor,

This is the automated single package builder at bioconductor.org.

Your package has been built on the Bioconductor Build System.

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 Bioconductor Build
System:
Linux (Ubuntu 22.04.2 LTS): gg4way_0.99.0.tar.gz
macOS 12.6.5 Monterey: gg4way_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/gg4way to trigger a new build.
A quick tutorial for setting up remotes and pushing to upstream can be found here.

@lshep lshep added the 2. review in progress assign a reviewer and a more thorough review of package code and documentation taking place label Sep 14, 2023
@bioc-issue-bot
Copy link
Collaborator

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 bioc-issue-bot removed the pre-review on bioconductor git and access to on demand build but not assigned reviewer until build report clean label Sep 14, 2023
@bioc-issue-bot
Copy link
Collaborator

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 bioc-issue-bot added the pre-review on bioconductor git and access to on demand build but not assigned reviewer until build report clean label Sep 14, 2023
@bioc-issue-bot
Copy link
Collaborator

Dear Package contributor,

This is the automated single package builder at bioconductor.org.

Your package has been built on the Bioconductor Build System.

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 Bioconductor Build
System:
Linux (Ubuntu 22.04.2 LTS): gg4way_0.99.0.tar.gz
macOS 12.6.5 Monterey: gg4way_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/gg4way to trigger a new build.
A quick tutorial for setting up remotes and pushing to upstream can be found here.

@bioc-issue-bot bioc-issue-bot added OK and removed ERROR labels Sep 14, 2023
@lshep lshep removed the pre-review on bioconductor git and access to on demand build but not assigned reviewer until build report clean label Sep 14, 2023
@bioc-issue-bot
Copy link
Collaborator

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 bioc-issue-bot added the pre-review on bioconductor git and access to on demand build but not assigned reviewer until build report clean label Sep 15, 2023
@bioc-issue-bot
Copy link
Collaborator

Dear Package contributor,

This is the automated single package builder at bioconductor.org.

Your package has been built on the Bioconductor Build System.

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 Bioconductor Build
System:
Linux (Ubuntu 22.04.2 LTS): gg4way_0.99.0.tar.gz
macOS 12.6.5 Monterey: gg4way_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/gg4way to trigger a new build.
A quick tutorial for setting up remotes and pushing to upstream can be found here.

@PeteHaitch
Copy link

Hi @ben-laufer,

Thank you for submitting gg4way to Bioconductor.
It's a nice, simple package with a clear purpose, which made for an straightforward review.
Thank you!

The package is largely fine as is, but I did want to ask whether you considered implementing gg4way() as a generic with specific methods for MArrayLM, DESeqDataSet, and (a list of) DESeqResults objects, along with a default version for the general 'tidyList' format?
The Glimma package does something similar (custom plots of DE analysis results) and uses the S3 object oriented programming system to implement this.
This would slightly simplify the user-interface and obviate the need to export the tidyDGE() function.
To be clear, this isn't a requirement for acceptance, but I thought it worth raising in case it is helpful to you.

In my review below I have separated the issues into Required and Recommended points that I would ask you to address before the package can be accepted.
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.

I'll be on leave (and offline) from today until October 9.
The deadline for new packages to be accepted is October 18 (see https://www.bioconductor.org/developers/release-schedule/).
I think the requested changes will be fairly straightforward for you implement and I will then be able to accept the package soon after returning from leave.
But if while I'm away you do have any urgent concerns or questions, please post to this issue and tag @vjcitn or @lshep to bring it to their attention.

Cheers,
Pete


Required

  • The man page for gg4way needs a little fleshing out. Some thoughts/questions I had when reading it are listed below. Some of these can be addressed with small changes to the current text, but I think the addition of a 'Details' section would be helpful.
    • The format of tidyList must be explicitly documented, particularly since it's not a formal class.
    • It could be made clearer that x and y must be the actual names of the results in the tidyList object rather than some names being added just for the purposes of axis labels on the plots.
    • Why are both symbol and ID required by gg4way()? It seems only symbol is used in the plot (as the optional labels).
    • Do you have any guidance on interpreting the correlation? For example, if the 2 contrasts 'overlap' (i.e. include a common factor), as they do in the vignette, then that would seem to induce a non-zero correlation whereas if the 2 contrasts do not overlap then I imagine the correlation does not have this 'bias'?
    • What does "sums" mean in "textSize Numeric specifying size of text with sums" and this "textOffset Numeric specifying offset of text with sums"?
    • What's actually plotted could be made more explicit. E.g., I assume it is the logFC from each comparison that's plotted on the x and y axes, but it's not actually stated.
  • In the vignette, the numbers sometimes differ between the text and figures, e.g., "71 DEGs" in text" vs. "72"" in bottom-left of first figure. Perhaps you can include the in-text values via inline evaluated R code (i.e. using r ) rather than hardcoding them.
  • All exported data in data/ must be documented, i.e. airwaysFit needs a man page; see https://contributions.bioconductor.org/citation.html.
  • The numbers reported in the quadrants seem to incorrect if a user forces all genes to be significant. Does this indicate deeper problems with this calculations or just a pathological corner case?
library(gg4way)
#> Loading required package: ggplot2

data("airwayFit")
airwayFit |>
    tidyDGE() |>
    gg4way(x = "N61311 vs N052611",
           y = "N061011 vs N052611",
           # Force all genes to be 'significant'.
           FDRcutoff = 1,
           logFCcutoff = 0)
#> Warning: Removed 1 rows containing missing values (`geom_label()`).

Session info
sessioninfo::session_info()
#> ─ Session info ───────────────────────────────────────────────────────────────
#>  setting  value
#>  version  R version 4.3.1 (2023-06-16)
#>  os       macOS Ventura 13.6
#>  system   aarch64, darwin20
#>  ui       X11
#>  language (EN)
#>  collate  en_US.UTF-8
#>  ctype    en_US.UTF-8
#>  tz       Australia/Melbourne
#>  date     2023-09-28
#>  pandoc   3.1.1 @ /Applications/RStudio.app/Contents/Resources/app/quarto/bin/tools/ (via rmarkdown)
#> 
#> ─ Packages ───────────────────────────────────────────────────────────────────
#>  package              * version   date (UTC) lib source
#>  abind                  1.4-5     2016-07-21 [1] CRAN (R 4.3.0)
#>  Biobase                2.61.0    2023-04-25 [1] Bioconductor
#>  BiocGenerics           0.47.0    2023-04-25 [1] Bioconductor
#>  BiocParallel           1.35.4    2023-08-17 [1] Bioconductor
#>  bitops                 1.0-7     2021-04-24 [1] CRAN (R 4.3.0)
#>  cli                    3.6.1     2023-03-23 [1] CRAN (R 4.3.0)
#>  codetools              0.2-19    2023-02-01 [1] CRAN (R 4.3.1)
#>  colorspace             2.1-0     2023-01-23 [1] CRAN (R 4.3.0)
#>  crayon                 1.5.2     2022-09-29 [1] CRAN (R 4.3.0)
#>  curl                   5.0.2     2023-08-14 [1] CRAN (R 4.3.0)
#>  DelayedArray           0.27.10   2023-07-28 [1] Bioconductor
#>  DESeq2                 1.41.11   2023-09-25 [1] Bioconductor
#>  digest                 0.6.33    2023-07-07 [1] CRAN (R 4.3.0)
#>  dplyr                  1.1.3     2023-09-03 [1] CRAN (R 4.3.0)
#>  evaluate               0.21      2023-05-05 [1] CRAN (R 4.3.0)
#>  fansi                  1.0.4     2023-01-22 [1] CRAN (R 4.3.0)
#>  farver                 2.1.1     2022-07-06 [1] CRAN (R 4.3.0)
#>  fastmap                1.1.1     2023-02-24 [1] CRAN (R 4.3.0)
#>  fs                     1.6.3     2023-07-20 [1] CRAN (R 4.3.0)
#>  generics               0.1.3     2022-07-05 [1] CRAN (R 4.3.0)
#>  GenomeInfoDb           1.37.4    2023-09-07 [1] Bioconductor
#>  GenomeInfoDbData       1.2.10    2023-03-26 [1] Bioconductor
#>  GenomicRanges          1.53.1    2023-05-04 [1] Bioconductor
#>  gg4way               * 0.99.0    2023-09-27 [1] Bioconductor
#>  ggplot2              * 3.4.3     2023-08-14 [1] CRAN (R 4.3.0)
#>  glue                   1.6.2     2022-02-24 [1] CRAN (R 4.3.0)
#>  gtable                 0.3.4     2023-08-21 [1] CRAN (R 4.3.1)
#>  htmltools              0.5.6     2023-08-10 [1] CRAN (R 4.3.0)
#>  IRanges                2.35.2    2023-06-23 [1] Bioconductor
#>  janitor                2.2.0     2023-02-02 [1] CRAN (R 4.3.0)
#>  knitr                  1.44      2023-09-11 [1] CRAN (R 4.3.1)
#>  labeling               0.4.3     2023-08-29 [1] CRAN (R 4.3.0)
#>  lattice                0.21-8    2023-04-05 [1] CRAN (R 4.3.1)
#>  lifecycle              1.0.3     2022-10-07 [1] CRAN (R 4.3.0)
#>  limma                  3.57.8    2023-09-24 [1] Bioconductor
#>  locfit                 1.5-9.8   2023-06-11 [1] CRAN (R 4.3.0)
#>  lubridate              1.9.3     2023-09-27 [1] CRAN (R 4.3.1)
#>  magrittr               2.0.3     2022-03-30 [1] CRAN (R 4.3.0)
#>  Matrix                 1.6-1.1   2023-09-18 [1] CRAN (R 4.3.1)
#>  MatrixGenerics         1.13.1    2023-06-09 [1] Github (Bioconductor/MatrixGenerics@8ecd537)
#>  matrixStats            1.0.0     2023-06-02 [1] CRAN (R 4.3.0)
#>  munsell                0.5.0     2018-06-12 [1] CRAN (R 4.3.0)
#>  pillar                 1.9.0     2023-03-22 [1] CRAN (R 4.3.0)
#>  pkgconfig              2.0.3     2019-09-22 [1] CRAN (R 4.3.0)
#>  purrr                  1.0.2     2023-08-10 [1] CRAN (R 4.3.0)
#>  R6                     2.5.1     2021-08-19 [1] CRAN (R 4.3.0)
#>  Rcpp                   1.0.11    2023-07-06 [1] CRAN (R 4.3.0)
#>  RCurl                  1.98-1.12 2023-03-27 [1] CRAN (R 4.3.0)
#>  reprex                 2.0.2     2022-08-17 [1] CRAN (R 4.3.0)
#>  rlang                  1.1.1     2023-04-28 [1] CRAN (R 4.3.0)
#>  rmarkdown              2.25      2023-09-18 [1] CRAN (R 4.3.1)
#>  rstudioapi             0.15.0    2023-07-07 [1] CRAN (R 4.3.0)
#>  S4Arrays               1.1.6     2023-08-31 [1] Bioconductor
#>  S4Vectors              0.39.2    2023-09-22 [1] Bioconductor
#>  scales                 1.2.1     2022-08-20 [1] CRAN (R 4.3.0)
#>  sessioninfo            1.2.2     2021-12-06 [1] CRAN (R 4.3.0)
#>  snakecase              0.11.1    2023-08-27 [1] CRAN (R 4.3.0)
#>  SparseArray            1.1.12    2023-08-31 [1] Bioconductor
#>  statmod                1.5.0     2023-01-06 [1] CRAN (R 4.3.0)
#>  stringi                1.7.12    2023-01-11 [1] CRAN (R 4.3.0)
#>  stringr                1.5.0     2022-12-02 [1] CRAN (R 4.3.0)
#>  SummarizedExperiment   1.31.1    2023-05-01 [1] Bioconductor
#>  tibble                 3.2.1     2023-03-20 [1] CRAN (R 4.3.0)
#>  tidyr                  1.3.0     2023-01-24 [1] CRAN (R 4.3.0)
#>  tidyselect             1.2.0     2022-10-10 [1] CRAN (R 4.3.0)
#>  timechange             0.2.0     2023-01-11 [1] CRAN (R 4.3.0)
#>  utf8                   1.2.3     2023-01-31 [1] CRAN (R 4.3.0)
#>  vctrs                  0.6.3     2023-06-14 [1] CRAN (R 4.3.0)
#>  withr                  2.5.1     2023-09-26 [1] CRAN (R 4.3.1)
#>  xfun                   0.40      2023-08-09 [1] CRAN (R 4.3.0)
#>  xml2                   1.3.5     2023-07-06 [1] CRAN (R 4.3.0)
#>  XVector                0.41.1    2023-05-03 [1] Bioconductor
#>  yaml                   2.3.7     2023-01-23 [1] CRAN (R 4.3.0)
#>  zlibbioc               1.47.0    2023-04-25 [1] Bioconductor
#> 
#>  [1] /Library/Frameworks/R.framework/Versions/4.3-arm64/Resources/library
#> 
#> ──────────────────────────────────────────────────────────────────────────────

Recommended

  • You might also add support for DGELRT objects from edgeR, the other major results format for DE analyses in Bioconductor.
  • That gg4way() can return a ggplot, numeric or tibble object depending on its arguments is a bit overcomplicated. Might it be better to have separate functions for the calculation of sharedOnly = TRUE and corOnly = TRUE results?
  • Any reason tidyList has to be a list of tibble objects rather than just data.frame objects?
  • I'll note that the limma/edgeR authors explicitly recommend against applying a logFC cutoff to the results of a DE analysis1, so gg4way(logFCcutoff = 1) is perhaps a 'bad' default.
  • Regarding the label argument of gg4way(), and this is a pathological case, but what if there was a gene symbol called "shared"? Perhaps use label = TRUE instead of label = "shared" to denote that option (since the default is shared = FALSE)?
  • Consider adding captions for figures in the vignette.
  • Perfectionist nitpicking: For the first figure in vignette, for example, that the %<-->% expression is not centered around x=0 (whereas it is for y=0) is slightly jarring. I'm not sure of a general solution to this, and so I apologise for even bringing it up, but I can't unsee it!
  • Once accepted, you could add a link from the README to the BioC landing page (which will be https://bioconductor.org/packages/gg4way/) and even the the rendered vignette (https://bioconductor.org/packages/release/bioc/vignettes/gg4way/inst/doc/gg4way.html) for where you write "See the package vignette." for examples.
  • Consider adding a inst/CITATION file.
  • Good job on including unit tests. Take a look at running covr::report() on your package for gaps in the test coverage.
  • Recommend running a spell check (if you use R Studio there is one built in). E.g., stop("Format not **suppourted**") in tidyDGE().

Footnotes

  1. "A commonly used approach is to apply FDR and logFC cutoffs simultaneously. However this tends to favor lowly expressed genes, and also fails to control the FDR correctly.".
    Instead, the authors developed limma::treat() and edgeR::glmTreat(); see their documentation for further details.
    DESeq2 may have similar functionality but I'm not as familiar with that package.

@bioc-issue-bot
Copy link
Collaborator

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

@bioc-issue-bot
Copy link
Collaborator

Dear Package contributor,

This is the automated single package builder at bioconductor.org.

Your package has been built on the Bioconductor Build System.

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 Bioconductor Build
System:
macOS 12.6.5 Monterey: gg4way_0.99.1.tar.gz
Linux (Ubuntu 22.04.2 LTS): gg4way_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/gg4way to trigger a new build.
A quick tutorial for setting up remotes and pushing to upstream can be found here.

@ben-laufer
Copy link
Author

Hi @PeteHaitch,

Thanks for reviewing gg4way. All the required changes have been made. See below for a point-by-point response about the improvements.

The package is largely fine as is, but I did want to ask whether you considered implementing gg4way() as a generic with specific methods for MArrayLM, DESeqDataSet, and (a list of) DESeqResults objects, along with a default version for the general 'tidyList' format? The Glimma package does something similar (custom plots of DE analysis results) and uses the S3 object oriented programming system to implement this. This would slightly simplify the user-interface and obviate the need to export the tidyDGE() function. To be clear, this isn't a requirement for acceptance, but I thought it worth raising in case it is helpful to you.

gg4way() is now a generic with specific methods for: MArrayLM, list, and DESeqDataSet. The list method can recognize lists of DGELRT, DESeqResults, and data.frame objects. The tidyDGE() function has been removed.

Required

  • The man page for gg4way needs a little fleshing out. Some thoughts/questions I had when reading it are listed below. Some of these can be addressed with small changes to the current text, but I think the addition of a 'Details' section would be helpful.

The man page for gg4way now clarifies these points.

  • The format of tidyList must be explicitly documented, particularly since it's not a formal class.

The tidyList argument has been renamed to DGEdata. The argument now contains details about the supported objects. The details section of the documentation also shows the format for a data.frame.

  • It could be made clearer that x and y must be the actual names of the results in the tidyList object rather than some names being added just for the purposes of axis labels on the plots.

This has been clarified in the argument descriptions.

  • Why are both symbol and ID required by gg4way()? It seems only symbol is used in the plot (as the optional labels).

The "other packages" section of the vignette (and function documentation) mention that the symbol column is optional and used for the plot labels. It enables cases where not every feature has a (unique) gene ID. The ID column is how unique features are defined.

  • Do you have any guidance on interpreting the correlation? For example, if the 2 contrasts 'overlap' (i.e. include a common factor), as they do in the vignette, then that would seem to induce a non-zero correlation whereas if the 2 contrasts do not overlap then I imagine the correlation does not have this 'bias'?

I have added a note about this to the details section of the man page for gg4way.

  • What does "sums" mean in "textSize Numeric specifying size of text with sums" and this "textOffset Numeric specifying offset of text with sums"?

The documentation for the arguments now mention that this is referring to the DEG totals for each category of gene overlap, which is shown by the numeric text in the plot.

  • What's actually plotted could be made more explicit. E.g., I assume it is the logFC from each comparison that's plotted on the x and y axes, but it's not actually stated.

Added to the description section of the man page and mentioned in the vignette.

  • In the vignette, the numbers sometimes differ between the text and figures, e.g., "71 DEGs" in text" vs. "72"" in bottom-left of first figure. Perhaps you can include the in-text values via inline evaluated R code (i.e. using r ) rather than hardcoding them.

In-text values are now obtained from inline R code.

Let me know if it's missing any key details.

  • The numbers reported in the quadrants seem to incorrect if a user forces all genes to be significant. Does this indicate deeper problems with this calculations or just a pathological corner case?

This has been fixed and additional error checks have been added.

Recommended

  • You might also add support for DGELRT objects from edgeR, the other major results format for DE analyses in Bioconductor.

Support for a list of DGELRT objects from edgeR has been added to gg4way() and an example is now provided in the vignette.

  • That gg4way() can return a ggplot, numeric or tibble object depending on its arguments is a bit overcomplicated. Might it be better to have separate functions for the calculation of sharedOnly = TRUE and corOnly = TRUE results?

Three extractor functions have been added, each of which return a distinct object. They are: getCor(), getShared(), and getTotals(). gg4way() will now only return a ggplot.

  • Any reason tidyList has to be a list of tibble objects rather than just data.frame objects?

The documentation now refers to a list of data.frame objects.

  • I'll note that the limma/edgeR authors explicitly recommend against applying a logFC cutoff to the results of a DE analysis1, so gg4way(logFCcutoff = 1) is perhaps a 'bad' default.

Support has been added for the output of the limma::treat() and edgeR::glmTreat() functions mentioned in the link as replacements for limma::eBayes() and edgeR::glmQLFTest(), respectively. The vignette now also mentions that gg4way() works with these and a similar approach in DESeq2. This should help with leaving the decision of the statistical approach up to the user. It also seems like the default of gg4way(logFCcutoff = 1) remains acceptable.

  • Regarding the label argument of gg4way(), and this is a pathological case, but what if there was a gene symbol called "shared"? Perhaps use label = TRUE instead of label = "shared" to denote that option (since the default is shared = FALSE)?

label = TRUE now replaces label = "shared".

  • Consider adding captions for figures in the vignette.

Added.

  • Perfectionist nitpicking: For the first figure in vignette, for example, that the %<-->% expression is not centered around x=0 (whereas it is for y=0) is slightly jarring. I'm not sure of a general solution to this, and so I apologise for even bringing it up, but I can't unsee it!

.tidyLabel() uses spacers to help with this, but it's only perfectly aligned when the limits of the axis are identical. The vignette details how a user can customize the axis titles through ggplot2 afterwards.

These links will be added upon acceptance.

The description file has been modified to generate a better default citation.

  • Good job on including unit tests. Take a look at running covr::report() on your package for gaps in the test coverage.

An additional unit test has been added.

  • Recommend running a spell check (if you use R Studio there is one built in). E.g., stop("Format not **suppourted**") in tidyDGE().

That one slipped through my R Studio spell check.

Footnotes

  1. "A commonly used approach is to apply FDR and logFC cutoffs simultaneously. However this tends to favor lowly expressed genes, and also fails to control the FDR correctly.".
    Instead, the authors developed limma::treat() and edgeR::glmTreat(); see their documentation for further details.
    DESeq2 may have similar functionality but I'm not as familiar with that package.

@PeteHaitch
Copy link

PeteHaitch commented Oct 12, 2023

I'm happy to mark gg4way as accepted.
Thank you for your engaging constructively in the review process, @ben-laufer, and for your contribution to Bioconductor!

A few final, minor things to address:

@PeteHaitch PeteHaitch added 3a. accepted will be ingested into Bioconductor daily builder for distribution and removed 2. review in progress assign a reviewer and a more thorough review of package code and documentation taking place pre-review on bioconductor git and access to on demand build but not assigned reviewer until build report clean labels Oct 12, 2023
@bioc-issue-bot
Copy link
Collaborator

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.

@lshep
Copy link
Contributor

lshep commented Oct 13, 2023

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

https://bioconductor.org/packages/gg4way

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 lshep closed this as completed Oct 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3a. accepted will be ingested into Bioconductor daily builder for distribution OK
Projects
None yet
Development

No branches or pull requests

5 participants