Feature/DIMS_previous_runs - #131
Conversation
| #' @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? (edit, for example is file is provided, but doesnot exist etc.)
There was a problem hiding this comment.
In case of no match with common_name there will be no error, just an empty rectangle in de violin plot. I don't think this need to be tested. If the file is not present, there will be an error in the main script.
ellendejong
left a comment
ellendejong
left a comment
There was a problem hiding this comment.
All review comments are resolved or discussed.
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.