Add plot_granges

[lee-t]

• add plot_shinygosling.R that adds the function plot_granges that takes a GRanges or GPos object formatted by AlphamissenseR and plots an interactive multiscale lolipop plot using shiny.gosling
• add examples for plot_granges to the vignette alphafold.Rmd
• updated DESCRIPTION with suggested packages and additional contributors.

[mtmorgan]

Thanks @lee-t for this really interesting addition! I've made a number of comments; several occur once but apply throughout the PR.

There are a number structural changes to the PR itself, but the most important for me was that at the end of the day the illustrative example did not really showcase the possibilities of Gosling-based visualization.

Out of scope for this PR, but I think it would be worth-while to explore an htmlwidget version of shiny.gosling; this seems to be more useful in terms of light-weight interactive widgets to be embedded in literate documents.

[mtmorgan]

These 'white space' changes replace spaces with tabs, but tabs are not used in AlphaMissesnseR. In general PRs should contain the essential changes required for the feature being implemented.

[lee-t]

resolved e6137b2

[mtmorgan]

sorry to be a pain but the original indentation was 8 spaces, not 4 and not tab.

[lee-t]

b0b5a45

[mtmorgan]

I think the argument should be designed to accept a GPos() object, which is the efficient way to store variant-style data. If a coercion to GRanges() is necessary (is it?) then this should be inside the function so that the user does not need to worry about this step.

I would name the argument gpos, which is more consistent with the argument names of other functions (e.g., tbl of as_GPos()).

[lee-t]

6dfd4b2

[mtmorgan]

The convention in AlphaMissenseR is to use character(1) to describe a scalar character, or character() if it can be any length.

[lee-t]

Resolved bbc4def

[mtmorgan]

This is not updated in the commit. I'm expecting something like

#' @param title the character(1) title of the plot <etc>

[lee-t]

aa46568

[mtmorgan]

there is no gosling package...

[lee-t]

resolved 7f44587

[mtmorgan]

Instead of \dontrun{}, I think it is better to run conditional on the necessary packages being available. In that way, the example, which can serve as a kind of integration test, is typically run on build systems, as well as when the user runs example(GPos_plot). Also no need to attach AlphaMissenseR (note the typo in line 31, not detected because the code is not run!) in the example. So

if (requireNamespace("GenomicRanges")) {
     ## rest of example code
}

[mtmorgan]

make this step unnecessary / compress creation of gpos to same as in example page.

[lee-t]

0c66dc9

[mtmorgan]

Use a more descriptive name instead of plot_example, e.g., GPos_plot

[lee-t]

0c66dc9

[mtmorgan]

No need to qualify as AlphaMissenseR::GPos_plot(), just GPos_plot().

At the end of the day the plot is not actually very interesting, and the formatting (with 1/2 points on the left, right, and bottom, and with the legend obscuring many points, at least for me) is not very good; is there a more compelling visualization that make better use of the display capabilities of shiny.gosling?

[lee-t]

b4a28bb

[mtmorgan]

Can you tell me what the consequences of this line are, especially for displaying the page in a pkgdown site like at https://mtmorgan.github.io/AlphaMissenseR ? If there are consequences, or if the intention is to provide further Gosling-based functionality, then does it make sense to introduce a new vignette, 'D. Gosling Integration'. For instance, perhaps like the AlphaFold vignette there is a 'fast path' that basically does everything for the user to created a canned visualization, and a more step-by-step illustrating construction of arbitrary Gosling artifacts.

[lee-t]

No obvious functionality differences on pkgdown because if you're publishing vignettes they are usually forced rendered as static HTML pages by knitr. The intention was if people did run the rmd then the document would start up the shiny runtime enabling the interactive

[mtmorgan]

Should some of these parameters be exposed as arguments for the user to customize? If so I'd write the function definition as something like GPos_plot(gpos, title, subtitle, ..., stroke = "lightgrey", <etc>) where more obscure arguments (including 'subtitle'?) go after the ... and follow as closely as possible whatever the shiny.gosling convention is.

[lee-t]

see the latest comment on the integration with gosling API

[mtmorgan]

Hi @lee-t I think we should either finish up the original pull request or close this while you work on this independently?

[lee-t]

Hi @mtmorgan
Thanks a lot for the comments, sorry it took this long to address them in one giant waterfall of responses. I want to take this time to explain the scope of the PR which is to provide the user with an interactive genome browser-like view on the AlphaMissense data for specfic gene of interest.
At this time, we (the team at CCB) do not intend to expose the entire Gosling API, ie the approach here is to hand-pick a couple of suitable plots for displaying the data, where a barplot-like view of the pathogenicity score at each position, similar to a sequencing coverage plot, seems to provide for a meaningful view. Lollipop plots focusing on the pathogenicity classification (ambiguous, benign, pathogenic) at each position would be another meaningful way to look at the data.
In the PR, we deploy gosling through shiny.gosling, a readily available Bioconductor package that wraps around a subset of gosling’s functionality through the R/Shiny framework. That is not optimal, but the most viable path to build upon at this point in time.
We agree that an htmlwidget implementation would be useful and an interesting project. In fact, Trevor Manz here from the Gehlenborg lab at HMS works on grosling, that provides an R-interface to the python version of gosling through reticulate, and that does return htmlwidgets. But this package is GitHub-only and not yet at a state that we could build on it in Bioconductor. We have spoken with Trevor about submitting grosling to Bioconductor, but this would take quite a bit more effort and time, and not something that he seemed to want to push like right now.
Based on your feedback I have now cleaned up code and the corresponding vignette section. One particular drawback of the shiny solution is that shiny apps can’t really be embedded into Bioc vignettes. Judged from other Shiny-based Bioc packages such as iSEE, the solution here seems to be to include screenshots of the app in the vignette, and only invoke the app when the user works in interactive mode (a24edaa). Please let us know whether you would support such a solution.

[mtmorgan]

Here the indentation when function arguments span more than one line is (also avoid 'cryptic' names where possible)

plot_granges <-
    function(
        granges,
        title = "GRanges Plot",
        subtitle = "Stacked nucleotide example"
    )
{
    ...

[lee-t]

73f173d

[mtmorgan]

Generally, combine assertions into a single stopifnot(). Use the S4 is() to test S4 objects (this might require a #' @importFrom methods is); I think the assertion is(granges, "GRanges") is sufficient for your purposes?. The convention in the package uses isScalarCharacter(), along the lines of (check that these are correct with respect to NAandnzchar()`...). I am conservative in use of additional error text, hoping to think that the message is sufficiently clear, or if not clear then encountered commonly enough that the user has figured out how to interpret it (rather than encountering different messages in each package).

stopifnot(
    is(granges, "GRanges"),
    isScalarCharacter(title),
    isScalarCharacter(subtitle),
    isScalarCharacter(plot_type)
)

Actually I think the plot_type test needs to be more explicit -- isScalarCharacter(plot_type) && plot_type %in% <allowable values>.

[lee-t]

57cbc97

[mtmorgan]

Please make sure that lines do not exceed 80 characters, following formatting conventions in the package, e.g.,

    gosling_cache <- file.path(
        R_user_dir("AlphaMissenseR", which = "cache"),
        ".gosling"
    )

[lee-t]

Outdated, see lines 76.

[mtmorgan]

As best practice use 'identical(plot_type, "bars"), since this returns TRUE or FALSE, whereas as written this can return cryptic errors if, e.g., plot_typewhere NA or "" (I realize that the checks above ensure that this is not the case, but that does not mean that the better practice of usingidentical()` should be avoided).

[lee-t]

c9e9088

[mtmorgan]

format so that the rendered example follows the convention of the package

#' plot_granges(
#'     gpos, mode = "bar", title = "Q1W6H_track",
#'     subtitle = "bar plot example"
#' )

[lee-t]

c65a351

[mtmorgan]

Thanks @lee-t ! I used a squash commit to merge your pull request (my opinion is that the commit history should be relatively 'clean', reflecting the final changes rather than the back-and-forth during code development). The squash commit enumerates all of your commits 45a7d1d but I am still trying to get this to be reflected in GitHub.

Also, I made three commits 448cc7e 79d6751 38b28b5 after yours that are perhaps worth reviewing, where I revised your code to a format that I thought was consistent with the package style. A big change involves naming the function gosling_plot().

[lee-t]

I reviewed the comments 448cc7e 79d6751 38b28b5 and think on the last one, helps a lot on the integration. Also wanted to express my gratitude for including the function in the package.