Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR cleans up package dependencies (removing several hard Imports), switches filesystem operations to base R, and updates reporting to primarily launch an external Shiny reporting app (PmetricsReports) with an HTML-render fallback.
Changes:
- Replace
fshelpers with basedir.create(),unlink(),file.exists(), etc., and remove related Imports. - Rework
PM_report()to launch thePmetricsReportsapp when available, with legacy HTML rendering as a fallback. - Replace
TruncatedNormal::rtmvnorm()usage with a new internal lower-bounded MVN sampler and update report templates/UI options accordingly.
Reviewed changes
Copilot reviewed 14 out of 15 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/testthat/test-pm-fit-prior-parameters.R | Switch test directory creation from fs::dir_create() to base dir.create(). |
| R/PMutilities.R | Replace openHTML/clear_build implementations and add internal PM_rtmvnorm() sampler. |
| R/PMoptions.R | Update options storage path creation and change reporting option to “app”-only. |
| R/Pmetrics-package.R | Remove roxygen @importFrom entries for dropped/softened dependencies. |
| R/PM_valid.R | Namespace-qualify kmeans() via stats::kmeans(). |
| R/PM_sim.R | Replace truncated MVN sampling callsites; large reformatting/realignment of code blocks. |
| R/PM_result.R | Replace fs::dir_create() with base dir.create() for binary copy path creation. |
| R/PM_report.R | Introduce “report app first” flow with fallback to Rmd rendering. |
| R/PM_model.R | Update run directory creation and reporting messages for “app launched” semantics. |
| R/PM_help.R | Remove rstudioapi usage; rely on environment variables for IDE version info. |
| NAMESPACE | Drop imports for packages moved out of Imports (e.g., DT, TruncatedNormal, lifecycle, mclust, npde). |
| man/PM_report.Rd | Update documentation to match new reporting behavior and parameter semantics. |
| inst/report/templates/ggplot.Rmd | Namespace DT usage and remove ggpubr::grids() dependency in plots. |
| inst/report/templates/ggplot_rust.Rmd | Same template updates as ggplot.Rmd. |
| DESCRIPTION | Remove several Imports (fs/DT/lifecycle/etc.) and add some to Suggests (including PmetricsReports). |
Files not reviewed (1)
- man/PM_report.Rd: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+1208
to
+1211
| d <- length(mean) | ||
| if (length(lb) != d) stop("lb length must equal length(mean)") | ||
| if (!all(dim(sigma) == c(d, d))) stop("sigma must be d x d") | ||
|
|
|
|
||
| draws <- draws + batch | ||
| if (draws > max_draws) { | ||
| stop("Exceeded max_draws before filling sample; truncation region may be too restrictive") |
Comment on lines
19
to
+41
| PM_report <- function(x, template, path, show = TRUE, quiet = TRUE) { | ||
| template_missing <- missing(template) | ||
| path_missing <- missing(path) | ||
|
|
||
| if (!is(x, "PM_result")) { | ||
| cli::cli_abort(c("x" = "This function expects a valid PM_result object from PM_load.")) | ||
| } | ||
|
|
||
| if (missing(template)) { | ||
| template <- getPMoptions("report_template") | ||
| if (!template_missing && identical(template, "none")) { | ||
| return(invisible(0)) | ||
| } | ||
|
|
||
| if (template == "none") { | ||
| return() | ||
| if (is.null(x$final$data) & is.null(x$op$data) & is.null(x$cycle$data)) { | ||
| return(invisible(-1)) # no data found | ||
| } | ||
|
|
||
| templateFile <- switch(template, | ||
| plotly = system.file("report/templates/plotly.Rmd", package = "Pmetrics"), | ||
| ggplot = system.file("report/templates/ggplot.Rmd", package = "Pmetrics"), | ||
| ggplot_rust = system.file("report/templates/ggplot_rust.Rmd", package = "Pmetrics") | ||
| ) | ||
| # templateFile = system.file("report/templates/ggplot.Rmd", package = "Pmetrics") | ||
| render_html_fallback <- function() { | ||
| fallback_template <- if (template_missing) getPMoptions("report_template") else template | ||
| if (identical(fallback_template, "none")) { | ||
| return(invisible(0)) | ||
| } | ||
| if (identical(fallback_template, "app") || is.null(fallback_template)) { | ||
| fallback_template <- "ggplot" |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Clean package dependencies, introduce report app