Feature/DIMS_QCinfo_in_mail #93
Conversation
BasMonkey
left a comment
BasMonkey
left a comment
There was a problem hiding this comment.
I've made some remarks about performance and code duplication. Please see the comments left on the code.
| file = paste(outdir, "sample_names_nodata.txt", sep = "/"), | ||
| row.names = FALSE, col.names = FALSE, quote = FALSE | ||
| ) | ||
| if (!is.null(sample_names_nodata)) { |
There was a problem hiding this comment.
This condition check seems redundant.
There was a problem hiding this comment.
Second if statement replaced by else condition.
| for (sample_name in sample_names_nodata) { | ||
| repl_pattern[[sample_name]] <- NULL |
There was a problem hiding this comment.
Right now, this for loop wil also run for the sample "none" if there is no data, which seems to be not the most elegant way. I would suggest to leave the value empty
There was a problem hiding this comment.
Problem solved by adding else condition in if statement.
| if (dims_matrix == "Plasma") { | ||
| # pos | ||
| for (line_index in seq_len(nrow(is_pos_selection_subset))) { | ||
| is_selected <- is_pos_selection_subset$HMDB_name[line_index] | ||
| thresh_selected <- all_is_thresholds$plasma$pos[which(all_is_thresholds$names$pos == is_selected)] | ||
| if (is_pos_selection_subset$Intensity[line_index] < thresh_selected) { | ||
| is_below_threshold <- rbind(is_below_threshold, is_pos_selection_subset[line_index, ]) | ||
| scanmode_is <- c(scanmode_is, "pos") | ||
| } | ||
| } | ||
| # neg | ||
| for (line_index in seq_len(nrow(is_neg_selection_subset))) { | ||
| is_selected <- is_neg_selection_subset$HMDB_name[line_index] | ||
| thresh_selected <- all_is_thresholds$plasma$neg[which(all_is_thresholds$names$neg == is_selected)] | ||
| if (is_neg_selection_subset$Intensity[line_index] < thresh_selected) { | ||
| is_below_threshold <- rbind(is_below_threshold, is_neg_selection_subset[line_index, ]) | ||
| scanmode_is <- c(scanmode_is, "neg") | ||
| } | ||
| } | ||
| } else if (dims_matrix == "DBS") { | ||
| # pos | ||
| for (line_index in seq_len(nrow(is_pos_selection_subset))) { | ||
| is_selected <- is_pos_selection_subset$HMDB_name[line_index] | ||
| thresh_selected <- all_is_thresholds$dbs$pos[which(all_is_thresholds$names$pos == is_selected)] | ||
| if (is_pos_selection_subset$Intensity[line_index] < thresh_selected) { | ||
| is_below_threshold <- rbind(is_below_threshold, is_pos_selection_subset[line_index, ]) | ||
| scanmode_is <- c(scanmode_is, "pos") | ||
| } | ||
| } | ||
| # neg | ||
| for (line_index in seq_len(nrow(is_neg_selection_subset))) { | ||
| is_selected <- is_neg_selection_subset$HMDB_name[line_index] | ||
| thresh_selected <- all_is_thresholds$dbs$neg[which(all_is_thresholds$names$neg == is_selected)] | ||
| if (is_neg_selection_subset$Intensity[line_index] < thresh_selected) { | ||
| is_below_threshold <- rbind(is_below_threshold, is_neg_selection_subset[line_index, ]) | ||
| scanmode_is <- c(scanmode_is, "neg") | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
This code is almost the same, except for the name change. I believe there is a more efficient way to achieve the same result. For example, move the for loop in a function, and pass the type as argument.
| # pos | ||
| for (line_index in seq_len(nrow(is_pos_selection_subset))) { | ||
| is_selected <- is_pos_selection_subset$HMDB_name[line_index] | ||
| thresh_selected <- all_is_thresholds$plasma$pos[which(all_is_thresholds$names$pos == is_selected)] |
There was a problem hiding this comment.
The which() method preforms a linear search action per row. A more efficient way is to make use of the match() method.
| is_selected <- is_pos_selection_subset$HMDB_name[line_index] | ||
| thresh_selected <- all_is_thresholds$plasma$pos[which(all_is_thresholds$names$pos == is_selected)] | ||
| if (is_pos_selection_subset$Intensity[line_index] < thresh_selected) { | ||
| is_below_threshold <- rbind(is_below_threshold, is_pos_selection_subset[line_index, ]) |
There was a problem hiding this comment.
Avoid rbind() in a loop, since it repeatedly reallocates and copies the data frame, which is inefficient and may use a huge amount of ram for larger datasets. Consider collecting indices or rows first and binding once at the end.
Deze feature zorgt ervoor dat er extra QC informatie vanuit de DIMS pipeline in de eindmail komt, zodat de gebruiker in 1 oogopslag de kwaliteit van de run kan beoordelen.
Verschillende stappen van de pipeline, met name AverageTechReplicates en GenerateQCOutput, genereren extra txt bestanden, die als content opgenomen worden in DIMS.nf.