Conversation
|
R/interface_module_qc_metrics.R
Outdated
| selectInput( | ||
| inputId = NS(id, "selected_assay"), | ||
| choices = NULL, | ||
| label = "Select set" |
There was a problem hiding this comment.
Select set for which to apply the dimension reduction
leopoldguyot
left a comment
leopoldguyot
left a comment
There was a problem hiding this comment.
Should we use the term "PCA" or "Dimension Reduction" ?
Dimension Reduction sounds better for what the page displays |
There was a problem hiding this comment.
Pull request overview
Refactors the QC metrics PCA UI/server logic to use a single “Dimension Reduction” plot driven by a shared set of parameters (method, orientation, scaling/centering, and coloring), instead of separate “features” and “samples” PCA boxes.
Changes:
- Adds a shared parameter panel (assay type, method, color-by, scaling/centering, legend, color label truncation length).
- Moves PCA color-choice population to
server_module_qc_metrics()and passes PCA parameters intoserver_module_pca_box()as reactives. - Simplifies
interface_module_pca_box()to only render the Plotly output (no dedicated box/sidebar inside the submodule).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| R/server_module_qc_metrics.R | Centralizes PCA parameter handling and updates server_module_pca_box() to accept reactives and compute samples/features PCA based on pca_type. |
| R/interface_module_qc_metrics.R | Reworks the QC metrics UI layout into a parameters box + single dimension reduction plot, and simplifies the PCA UI submodule. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| selectInput( | ||
| inputId = NS(id, "assay_type"), | ||
| choices = c("samples", "features"), | ||
| label = "Select dimension reduction type", | ||
| selected = "samples" | ||
| ), |
There was a problem hiding this comment.
interface_module_qc_metrics() receives a type argument (passed by interface_module_filtering_tab() as either "samples" or "features"), but the new assay_type selector is hard-coded to selected = "samples" and the type argument is otherwise unused. This makes the default PCA orientation inconsistent with the filtering tab type. Consider validating type via match.arg() and using it as the default selected value (or remove the type parameter entirely if it’s no longer intended to control the UI).
| #' @importFrom methods is | ||
| #' | ||
| server_module_pca_box <- function(id, single_assay, method, transpose) { | ||
| server_module_pca_box <- function(id, single_assay, method, pca_type, scale, center, show_legend, color, color_width) { |
There was a problem hiding this comment.
The roxygen documentation for server_module_pca_box() is now out of sync with the function signature and behavior (it still documents transpose and says only "nipals" is valid, but the function now takes pca_type, scale, center, show_legend, color, and color_width, and transpose is derived from pca_type()). Please update the @param entries to match the new arguments and semantics.
| interface_module_pca_box <- function(id) { | ||
| with_output_waiter(plotlyOutput(outputId = NS(id, "pca")), | ||
| html = waiter::spin_6(), | ||
| color = "transparent" | ||
| ) | ||
| } |
There was a problem hiding this comment.
The roxygen documentation for interface_module_pca_box() is outdated: it still documents a title parameter and describes returning a box/boxSidebar, but the function now only returns the plotlyOutput wrapped with with_output_waiter(). Please update/remove the stale @param title and adjust the return description/import tags accordingly.
Not sure about color_width button, should we remove it ?