-
Notifications
You must be signed in to change notification settings - Fork 5
Create R script to replicate App outputs (#467) #789
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
base: main
Are you sure you want to change the base?
Conversation
…atio calculations
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR implements an R script export feature that allows users to download an executable R script capturing their session configuration. The script can be run independently to reproduce the same NCA analysis results.
Key changes:
- Added
get_session_script_code()function to generate executable R scripts from templates with session data substitution - Moved and exported
calculate_table_ratios_app()function from Shiny-specific code to the package API - Added session data capture in multiple Shiny modules to support script generation
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
R/get_session_script_code.R |
New function to parse templates and substitute session variables with actual values |
R/ratio_calculations.R |
Added exported calculate_table_ratios_app() function (moved from internal Shiny code) |
inst/shiny/www/templates/script_template.R |
Template for generating session-specific R scripts with placeholders for session data |
inst/shiny/modules/tab_nca/nca_results.R |
Integration of script generation into the export ZIP functionality |
inst/shiny/modules/tab_nca.R |
Captures settings, ratio_table, and slope_rules to session userData |
inst/shiny/modules/tab_data/*.R |
Captures mapping, filters, and data paths to session userData |
inst/shiny/modules/tab_nca/setup.R |
Removed duplicate ratio_table assignment |
inst/shiny/functions/ratio_table.R |
Removed calculate_table_ratios_app() (moved to R/ratio_calculations.R) |
man/*.Rd |
Documentation files for new and moved functions |
NAMESPACE |
Export declaration for calculate_table_ratios_app() |
inst/WORDLIST |
Added technical terms for spell checking |
R/zzz.R |
Added calculate_ratio_app to global variables list |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review for a chance to win a $100 gift card. Take the survey.
| warning( | ||
| "Ratio ", ratio_table$PPTESTCD[i], " not computed.", | ||
| "No comparable groups found between RefGroups", | ||
| " (", ratio_table$RefGroups[i], ")", | ||
| "and TestGroups", | ||
| " (", ratio_table$TestGroups[i], ")" | ||
| ) |
Copilot
AI
Nov 28, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The warning message is missing spacing between sentences. Add a space after "not computed." to improve readability: "Ratio ", ratio_table$PPTESTCD[i], " not computed. No comparable groups found..."
apply relevant copilot suggestions Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
I opened a new issue (#886) to address this apart, as it will need to modify further more other files from main
The differences I saw where DTYPE and the duplicates. So it would be related again with #886
Now it has been corrected in the last commits
This is expected to happen when changing between dev-branches. This shouldn't present a problem to the user and neither to us once this change is stabilized. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, but currently the function needs the session object, so you have to go through the app anyway? I would agree if the function also accepted a settings file and had a more generic name, but currently it does not.
At the very least, if we want to implement it this way right now and then add the functionality of parsing a settings file instead of session object, we should have
- more generic name
# TODOcomments with link or number of appropriate issue to rely that intent.
R/get_session_code.R
Outdated
| if (nrow(obj) == 0) { | ||
| return("data.frame()") | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nitpick: This is redundant, as it is handled by lines 80-83
if (length(obj) == 0 && !is.null(obj)) {
return(paste0(class(obj)[1], "()"))
}
R/ratio_calculations.R
Outdated
| #' AdjustingFactor, TestGroups, RefGroups, PPTESTCD. | ||
| #' @returns The updated PKNCAresult object with added rows in the `result` data.frame. | ||
| #' @export | ||
| calculate_table_ratios_app <- function(res, ratio_table) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
R/ratio_calculations.R
Outdated
| ratio_results <- vector("list", nrow(ratio_table)) | ||
|
|
||
| # Loop through each row of the ratio_table | ||
| for (i in seq_len(nrow(ratio_table))) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion: Use apply instead of a for loop.
R/ratio_calculations.R
Outdated
|
|
||
| all_ratios <- data.frame() | ||
|
|
||
| for (ix in seq_along(match_cols)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion: use apply or some other mapping columns, create a list and then bind all rows at once instead of adding rows to an empty data frame one by one.
I created a TODO and the implementation will be developed in #835. The name/refactoring of this function I will be done there just for easiness. Here, in this PR, I removed the redundant check, renamed |
|
LGTM :) |
Shaakon35
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here we go!! Congratulations @Gero1999 👍
|
@Gero1999 Can you add in the README, in the Description bullet list:
|
Merge branch '467-enhancement/r-script' of https://github.com/pharmaverse/aNCA into 467-enhancement/r-script # Conflicts: # DESCRIPTION
|
Issue with flags was partially solved here (de-selection of rules prevent color reporting of rows): #896 |
|
I will rework on this to properly incorporate the new aspects integrated in
I will include quick |
Issue
Closes #467
Description
Implemented the script file. It was made in order to eventually also be used using a settings file
Definition of Done
How to test
Do your choices in the App and export the ZIP folder. In the subfolder find the
/code/session_script.Rand run it. Compare the script outputs with the result files in the exported ZIPContributor checklist
Notes to reviewer
calculate_ratios_appwill be included here Enhancement: Add tests for functions to reach 100% coverage #287 independentlyget_session_script_codehas probably to be also done as an e2e test. To indeed make sure that both session and script are matching