Feature/DIMS_previous_runs - #131
Conversation
| add_previous_runs <- function(patient_zscore_df, data_previous_runs) { | ||
| data_previous_runs <- as.data.frame(data_previous_runs) | ||
| for (row_nr in 1:nrow(patient_zscore_df)) { | ||
| # match data from previous run to data from current run based on common name |
There was a problem hiding this comment.
Start with capital letter in comments.
https://style.tidyverse.org/documentation.html#capitalization-and-full-stops
In case you used lowercase all the time, consider this remark out-of-scope for this PR. In that case it is better to be consistent. However, you might want to consider changing this across whole project later on. :)
Just out of curiosity, any linter/styleguide/formatter check used by default?
There was a problem hiding this comment.
There is a separate feature branch for harmonizing all R scripts, see #130
in which many things have been standardized. I didn't check all comments in all R files though, but I intend to do linting and style checks after all feature branches for DIMS v3.5 have been properly merged.
| patient_zscore_df$max[row_nr] <- data_previous_runs$max[find_rownr] | ||
| patient_zscore_df$min95[row_nr] <- data_previous_runs$min95[find_rownr] | ||
| patient_zscore_df$max95[row_nr] <- data_previous_runs$max95[find_rownr] | ||
| } |
There was a problem hiding this comment.
I wonder if you need 5 separate lines, or whether the assignments can be combined;
patient_zscore_df[row_nr, c("mean", "min", "max", "min95", "max95")] <-
data_previous_runs[find_rownr, c("average_patients", "min", "max", "min95", "max95")]
There was a problem hiding this comment.
The 5 lines can be replaced by 2 lines; modified.
| common_name <- trimws(patient_zscore_df$HMDB_name[row_nr]) | ||
| find_rownr <- which(data_previous_runs$common_name == common_name) | ||
| # add mean, min, max and 5-95% interval of data from previous runs | ||
| if (length(find_rownr) == 1) { |
There was a problem hiding this comment.
Correct me if I am wrong. I assume this conditional is to only add data if common_name is found (not 0). Theoretically, it is possible to have a find_rownr > 1. Do you need to check for this as well, and maybe add multiple values (or average or ...)?
There was a problem hiding this comment.
The dataframe data_previous_runs doesn't contain any double entries, so find_rownr is either 0 or 1. In case this situation does occur in the future, the current code will not give a rectangle in the violin plot which will be detectable.
| #' @param explanation: text that explains the violin plots and the pipeline version (string) | ||
| create_pdf_violin_plots <- function(pdf_dir, patient_id, metab_perpage, top_metab_pt, explanation) { | ||
| #' @param data_previous_runs: data from previous DIMS runs, not used for dIEM plots (matrix) | ||
| create_pdf_violin_plots <- function(pdf_dir, patient_id, metab_perpage, top_metab_pt, explanation, data_previous_runs = NULL) { |
There was a problem hiding this comment.
In my opinion, create_pdf_violin_plots is quite long (large number of lines). Adding additional code to it, makes me wonder if it could be split into multiple methods.
For example:
- A method that determines the PDF filename.
- Any edits to the data itself. (filter/select, mutates etc)
- Create a list (or something similar) with all plots.
- Create the page layout with tableGrob + gridarrange etc using the plot list.
- Write to PDF. (Or, combine with determining the PDF filename)
Would like to hear your opinion. :)
There was a problem hiding this comment.
The part that determines the pdf file name could easily be made into a separate function. Creating a list of plots would not be my preference, I would prefer to keep each type of plot as a separate unit.
This code has been refactored previously and will remain as is for v3.5.
| file.remove("missing_probability_scores.txt") | ||
| }) | ||
|
|
||
| testthat::test_that("add_previous_runs: Information for metabolites from previous runs is correctly added", { |
There was a problem hiding this comment.
Is a 'negative' test also relevant? Aka In case no match with common_name, or no data present.
Are there any other edge cases to cover? If an input file could have metadata from multiple runs, you could test a scenario with single and once with multiple runs.
| file_ratios_metabolites <- cmd_args[4] | ||
| file_expected_biomarkers_iem <- cmd_args[5] | ||
| file_explanation <- cmd_args[6] | ||
| file_previous_runs <- cmd_args[7] |
There was a problem hiding this comment.
Are there multiple runs in a single file?
There was a problem hiding this comment.
file_previous_runs contains summarized data (min, max, mean) from runs from the past half a year in one dataframe.
| #' @param explanation: text that explains the violin plots and the pipeline version (string) | ||
| create_pdf_violin_plots <- function(pdf_dir, patient_id, metab_perpage, top_metab_pt, explanation) { | ||
| #' @param data_previous_runs: data from previous DIMS runs, not used for dIEM plots (matrix) | ||
| create_pdf_violin_plots <- function(pdf_dir, patient_id, metab_perpage, top_metab_pt, explanation, data_previous_runs = NULL) { |
There was a problem hiding this comment.
Outside the scope of this PR. When I read the full function, I noticed table_theme is declared twice, line 641 and 679. I don't see any differences between the two lines. Is it possible that 679 could reuse the declared theme?
There was a problem hiding this comment.
This has been addressed in feature/DIMS_DrugDB.
2 participants
Added rectangles in the violin plots to indicate the 5-95% range of results for patients for each metabolite in previous runs.
Extra file is imported in GenerateViolinPlots and generate_violin_plots_functions has new function add_previous_runs. Unit test for new function has been added.