Feature/DIMS_Excel_plots #92
Conversation
…styler and linter
rernst
left a comment
rernst
left a comment
There was a problem hiding this comment.
Ziet er goed uit, ik denk dat je naamgeving soms wat duidelijker/simpeler kan maken. Verder zit er nog een stukje code duplicatie in wat je volgens mij bij de create_boxplot_excel functie kan toevoegen.
| sorted_control_intensities <- sort(as.numeric(control_intensities)) | ||
| trimmed_control_intensities <- sorted_control_intensities[(nr_to_remove + 1) : | ||
| trimmed_control_intensities <- sorted_control_intensities[(nr_to_remove + 1): | ||
| (length(sorted_control_intensities) - nr_to_remove)] |
| #' @param intensities_plots_df: a dataframe with HMDB_key column and intensities for all samples | ||
| #' | ||
| #' @returns intensities_plots_df_long: a dataframe with on each row a sample and their intensity | ||
| transform_ints_df_plots <- function(intensities_plots_df, row_index) { |
There was a problem hiding this comment.
Ik zou het woord 'plots' uit de functie naam halen, functie maakt namelijk geen plots.
Gezien je docstring header zou ik de functie noemen:
transform_intensities_df_to_long_format of intensities_df_to_long_format.
Interne variable intensities_plots_df zou ik dan ook intensities_df noemen en intensities_df_long.
| #' @param hmdb_id: HMDB ID of the selected metabolite | ||
| #' | ||
| #' @returns boxplot_excel: ggplot2 object containing the plot of intensities | ||
| create_boxplot_excel <- function(intensities_plots_df_long, hmdb_id) { |
There was a problem hiding this comment.
Ook hier zo ik van maken: create_boxplot. Excel toevoegingen zijn niet echt nodig omdat het een functie is van de generate_excel_functions.R. Eventueel zou je hem 'private' kunnen maken. In python zou je dan doen: _create_boxplot -> dat geeft aan dat het niet de bedoeling is om de functie buiten de context van deze file te gebruiken. Weet niet of dat een ding is in de R 'wereld'?
| # get HMDB ID | ||
| hmdb_name <- rownames(outlist[row_index, ]) | ||
| # Get intensity columns for controls and patients | ||
| intensities_plots_df <- outlist %>% select(HMDB_key, matches("^C|^P[0-9]"), -ends_with("_Zscore")) |
There was a problem hiding this comment.
Ook hier intensities_plots_df -> intensities_df het is immers een df van intensities niet een df van intensities plots.
| plot.new() | ||
| tmp_png_helix <- paste0("plots/plot_helix_", hmdb_id, ".png") | ||
| png(filename = tmp_png_helix, width = plot_width, height = 300) | ||
|
|
||
| boxplot_excel_helix <- create_boxplot_excel(intensities_plots_df_long, hmdb_id) | ||
|
|
||
| print(boxplot_excel_helix) | ||
| dev.off() |
There was a problem hiding this comment.
Zou dit ook niet in de 'create_boxplot_excel' functie passen? Het lijkt er nu op de het maken van de boxplot feitelijk nog hier gebeurd, create boxplot genereerd nu enkel de ggplot config?
| boxplot_excel <- create_boxplot_excel(intensities_plots_df_long, hmdb_id) | ||
|
|
||
| print(boxplot_excel) | ||
| dev.off() |
There was a problem hiding this comment.
Hier wordt de code van hierboven vrijwel hetzelfde nog een keer gebruikt?
Uh oh!
There was an error while loading. Please reload this page.