Issue 31: Update mzroll_multi_qc function #43
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR addresses Issue 31 by updating the mzroll_multi_qc() function to handle cases where not all samples have detected compounds in all methods. It replaces the previous internal function that was causing errors and adds a new qc_strict parameter to control error handling behavior.
Key changes:
- Deprecated the old
mzroll_multi_qc()function and created a new implementation with improved error handling - Added
qc_strict = TRUEparameter toprocess_mzroll_multi()for backwards compatibility - Fixed a spelling error in documentation ("principle" → "principal")
Reviewed Changes
Copilot reviewed 5 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| R/import_mzroll.R | Added new mzroll_multi_qc() function with qc_strict parameter and moved old implementation to mzroll_multi_qc_old() |
| man/process_mzroll_multi.Rd | Added documentation for new qc_strict parameter |
| man/check_outliers.Rd | Fixed spelling error in documentation |
| R/utils.R | Added @export directive to test_mzroll_list() function |
| NAMESPACE | Updated to export test_mzroll_list function |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
R/import_mzroll.R
Outdated
|
|
||
| mzroll_multi_qc <- function(mzroll_list) { | ||
| mzroll_multi_qc <- function(mzroll_list, | ||
| qc_strict = qc_strict) { |
There was a problem hiding this comment.
The parameter qc_strict is being assigned to itself. This should reference the parameter passed to the function, not itself. Consider renaming the parameter to avoid confusion or use a different default value.
| qc_strict = qc_strict) { | |
| qc_strict = TRUE) { |
R/import_mzroll.R
Outdated
| nrow(missed_matches), " rows only matched a subset of methods.", | ||
| "\nName(s) of probematic samples:\n", | ||
| missed_matches_w_data$message_out, "", | ||
| "\nAggregrating mzrollDB, but mzroll_list has potential downstream problems." |
There was a problem hiding this comment.
There is a spelling error: 'Aggregrating' should be 'Aggregating'.
| "\nAggregrating mzrollDB, but mzroll_list has potential downstream problems." | |
| "\nAggregating mzrollDB, but mzroll_list has potential downstream problems." |
| quant_col = "peakAreaTop") { | ||
| quant_col = "peakAreaTop", | ||
| qc_strict = TRUE) { | ||
| checkmate::assertDataFrame(mzroll_paths, min.rows = 2) |
There was a problem hiding this comment.
This is a key change--can change qc_strict = FALSE which allows the aggregated mzroll_list to be passed through the function, even if not all samples are detected in both modes. Because this defaults to TRUE, everything will handle the same as before on default.
PMSeitzer
left a comment
PMSeitzer
left a comment
There was a problem hiding this comment.
LGTM, it looks like copilot caught some bugs, I'd recommend following those suggestions
Description of your changes
Depreciate the previous internal
mzroll_multi_qc()function which was causing errorsCreate new
mzroll_multi_qc()function which uses correct metadata column names, and permits passing of aggregated mzroll_list, even when not all samples have detected compounds in all modesAs part of this change, add
qc_strict = TRUEdefault toprocess_mzroll_multi(), which is backwards compatible and will operate as before unless manually changed.Issue ticket number and link
#31 as part of pipeline update for calico/mass_spec#1786
Type of change
(If applicable) How has this been tested?