Internal MGF export functions gain parameter addFields #362
Conversation
- Add parameter addFields to internal writeMgfDataFile and writeMgfContent to support addition of arbitrary additional fields in the MGF output file. - Add related unit tests.
R/readWriteMgfData.R
Outdated
| if (class(con) == "character" && file.exists(con)) { | ||
| message("Overwriting ", con, "!") | ||
| unlink(con) | ||
| } | ||
|
|
||
| if (length(addFields)) { | ||
| if (is.null(dim(addFields))) |
There was a problem hiding this comment.
Why you don't use is.matrix(addFields) || is.data.frame(addFields) (I guess because you want to support S4Vectors::DataFrame and so on)? But than I think length(dim(addFields)) == 2 would be safer. I don't know why anybody want to try this but currently your code accepts addFields = array(1:27, dim=c(3, 3, 3)). as.matrix in the following line will turn it in a 27x1 matrix which will (hopefully) cause an error at nrow(addFields) != length(splist).
There was a problem hiding this comment.
Good point @sgibb ! Thanks! I'll fix.
R/readWriteMgfData.R
Outdated
| stop("Column names required on 'addFields'.") | ||
| if (nrow(addFields) != length(splist)) | ||
| stop("nrow of 'addFields' has to match length of 'splist'") | ||
| haveAddFields <- TRUE |
There was a problem hiding this comment.
Do you need this additional variable haveAddFields? Would not is.null(addFields) be enough in line 63 (if (haveAddFields)). But even this would not be needed at all (see below).
There was a problem hiding this comment.
No, we don't. Usually I add a variable instead of calling is.null 2 or more times on the same object in a function. Doesn't apply here, but if the function would be called in a loop this can have a performance effect.
But good point with the NULL subsetting! Actually never tried subsetting NULL 😄
R/readWriteMgfData.R
Outdated
| @@ -45,13 +60,17 @@ writeMgfDataFile <- function(splist, con, COM = NULL, TITLE = NULL, | |||
| if (verbose) | |||
| setTxtProgressBar(pb, i) | |||
|
|
|||
| writeMgfContent(splist[[i]], TITLE = NULL, con = con) | |||
| if (haveAddFields) | |||
There was a problem hiding this comment.
Do we really need this if statement here. If we reach this point addFields is a matrix or NULL. And you can subset NULL:
f <- function(n, addFields=NULL) {
for (i in seq_len(n)) {
print(addFields[i, ])
}
}
f(3)
# NULL
# NULL
# NULL
f(3, matrix(1:6, ncol=2, dimnames=list(NULL, c("FOO", "BAR"))))
# FOO BAR
# 1 4
# FOO BAR
# 2 5
# FOO BAR
# 3 6 While it looks a little bit odd/dirty the workhorse function writeMgfContent will test for NULL itself.
R/readWriteMgfData.R
Outdated
| } | ||
| if (verbose) | ||
| close(pb) | ||
| } | ||
|
|
||
| writeMgfContent <- function(sp, TITLE = NULL, con) { | ||
| writeMgfContent <- function(sp, TITLE = NULL, con, addFields = TRUE) { |
There was a problem hiding this comment.
I don't understand the default value TRUE for addFields here.
There was a problem hiding this comment.
Ups, well, me neither. I'll change to addFields = NULL.
R/readWriteMgfData.R
Outdated
| @@ -83,6 +102,11 @@ writeMgfContent <- function(sp, TITLE = NULL, con) { | |||
| } | |||
| } else .cat("\nRTINSECONDS=", rtime(sp)) | |||
|
|
|||
| if (length(addFields)) { | |||
There was a problem hiding this comment.
Instead of the nested if statement: length(addFields) && !is.null(names(addFields)) or is.vector(addFields) && !is.null(names(addFields))?
There was a problem hiding this comment.
good point. Will change.
R/readWriteMgfData.R
Outdated
| if (length(addFields)) { | ||
| if (!is.null(names(addFields))) | ||
| .cat(paste0("\n", paste(toupper(names(addFields)), | ||
| addFields, sep = "="), collapse = "")) |
There was a problem hiding this comment.
.cat("\n", paste(toupper(names(addFields)), addFields, sep = "=", collapse = "\n"))
There was a problem hiding this comment.
yes, better that way. Thanks.
| if (length(dim(addFields)) != 2) | ||
| stop("'addFields' has to be a matrix or data.frame.") | ||
| if (!is.matrix(addFields)) | ||
| addFields <- do.call(cbind, lapply(addFields, as.character)) |
There was a problem hiding this comment.
I really hate as.matrix for calling format and padding all characters with whitespaces. Instead of do.call and lapply we could use addFields <- trimws(as.matrix(d)) here.
There was a problem hiding this comment.
BTW: Internal as.matrix.data.frame uses a for loop. So depends on your style whether you like the do.call, cbind, lapply or trimws(as.matrix()) approach.
There was a problem hiding this comment.
I'd rather take the do.call, cbind, lapply avenue - it's faster then the trimws(as.matrix). But thanks for the suggestion.
|
👍 |
Add parameter addFields to internal writeMgfDataFile and writeMgfContent to support addition of arbitrary additional fields in the MGF output file.