Conversation
…tor_GenerateViolinPlots
rernst
left a comment
rernst
left a comment
There was a problem hiding this comment.
First of all lots of work done, good job! I feel like there is room for improvement, some general thoughts:
-
Many parameters have names like metab_interest_sorted. In the context of a function, it’s not relevant whether the input is “of interest” or “sorted.” Use neutral, descriptive names that reflect the data type or role.
-
Several functions are named after their use case rather than their functionality. name functions based on what they do, not where they are used.
-
When breaking function calls across lines, maintain a consistent style. Preferred format:
Rfunction1(
function_2(param_a),
param_b,
param_c,
)-
There is no error catching for missing files or invalid paths. Currently, the code will crash, making debugging difficult.
-
There seems to be a lot of ad-hoc data transformations. It feels like the DIMS application is missing a standardized data format for saving and reusing data between steps.
This reverts commit 78285ff. Changes to the wrong feature branch.
rernst
left a comment
rernst
left a comment
There was a problem hiding this comment.
Awesome progress, just two comments. I did check only changed code compared to last review.
Secondly, we had discussion about docstrings in functions, looking at this code i personally realy prefer to have docstrings 'part' of the function not above...
And not for this PR, but you might want to take a look at https://covr.r-lib.org -> this can generate test coverage files which makes it easy to verify which code is tested. Personally I tent to focus on the 'regular' code and would just like to see test coverage.
| sample_intersect <- intersect(paste0(intensity_cols, "_Zscore"), grep("_Zscore", colnames_zscore, value = TRUE)) | ||
| return(sample_intersect) | ||
| #' @returns sample_colnames: a vector of column names all containing the prefix. | ||
| get_colnames_samples <- function(dataframe, sample_label) { |
There was a problem hiding this comment.
Rename to get_colnames_by_prefix, and sample_label => prefix.
Furthermore, this function now have sort of hidden functionally of also filtering '_Zscore' columns. I would suggests to or create a separate function to remove the suffix or change the name to reflect this behaviopur. I would prefer the first option.
For example:
sample_colnames = remove_suffix_from_items(get_colnames_by_prefix(df, 'P'), "_Zscore")
| list_of_dataframes <- lapply(txt_files_paths, | ||
| read.table, | ||
| sep = "\t", header = TRUE, quote = "" | ||
| ) |
2 participants
The refactor of GenerateViolinPlots: