Conversation
TuomasBorman
left a comment
TuomasBorman
left a comment
There was a problem hiding this comment.
Some suggestions. Code looks already simple and good, just like it should be.
| BiocStyle, | ||
| knitr, | ||
| RefManageR, | ||
| MultiAssayExperiment, |
There was a problem hiding this comment.
Is MultiAssayExperiment needed? Try to keep the dependency list as short as possible.
| setMethod( | ||
| "getTtest", "SummarizedExperiment", | ||
| function (x, assay.type = NULL, row.var = NULL, col.var = NULL, | ||
| formula, split.by = NULL, pair.by = NULL, features = NULL, | ||
| var.equal = FALSE, p.adjust.method = "fdr", ... ){ | ||
| ############################# Input check ############################## | ||
| group <- .check_input( |
There was a problem hiding this comment.
Run:
styler::style_file(path = "inst/pages/machine_learning.qmd", transformers = styler::tidyverse_style(indent_by = 4L))
| .is_a_bool <- mia:::.is_a_bool | ||
| .is_non_empty_string <- mia:::.is_non_empty_string | ||
| .is_non_empty_character <- mia:::.is_non_empty_character | ||
| .check_assay_present <- mia:::.check_assay_present | ||
| .check_and_get_altExp <- mia:::.check_and_get_altExp | ||
| .add_values_to_colData <- mia:::.add_values_to_colData |
There was a problem hiding this comment.
I might even drop mia from dependency... We can discuss this in the future.
| # It must be a string and found from colData/rowData | ||
| is_string <- ifelse(multiple, is.character(var), .is_non_empty_string(var)) | ||
| check_values <- c() | ||
| check_values <- c(check_values, if( col) colnames(colData(tse))) |
There was a problem hiding this comment.
if( col) -> remove odd space
| # Return group for use in caller | ||
| group | ||
| } |
There was a problem hiding this comment.
End every function with return(). Check also other functions.
| out <- data.frame( | ||
| .y. = y, | ||
| group1 = NA_character_, | ||
| group2 = NA_character_, | ||
| n1 = NA_integer_, | ||
| n2 = NA_integer_, | ||
| statistic = NA_real_, | ||
| p = NA_real_, | ||
| warning = msg | ||
| ) |
There was a problem hiding this comment.
This might be problematic as the idea is to use this same .calc_daa method for other tests. By doing so, we can support lots of different tests by using same infrastructure. Those other statistical tests might not returns same set of columns.
Can you do it so that it works for all columns, i.e., does not have these fixed columns for failed results.
| if( "p" %in% colnames(res) && any(!is.na(res$p))) { | ||
| res <- res |> adjust_pvalue(method = p.adjust.method) | ||
| } else { | ||
| res$p.adj <- NA_real_ |
There was a problem hiding this comment.
Might be more clear if p.adj column is only added to results if p-values were adjusted
| .calc_daa <- function(df, y, group, split.by, paired, FUN, | ||
| p.adjust.method = "fdr", features = NULL, ...) { |
There was a problem hiding this comment.
I am wondering if there is use case for split.by...
Although, it might be nice to have...
| res <- df |> | ||
| group_split(across(all_of(grouping_vars))) |> |
There was a problem hiding this comment.
Can this be achieved using
df <- df |> group_by()
Then the code would be something like this:
if( length(grouping_vars) > 0) {
df <- df |> group_by()
}
if( paired && !is.null(pair.by)) {
df <- df |> arrange(across(all_of(pair.by)))
}
res <- FUN(df, formula, paired = paired, ...)
| function(x, assay.type = NULL, row.var = NULL, col.var = NULL, | ||
| formula, split.by = NULL, pair.by = NULL, features = NULL, |
There was a problem hiding this comment.
formula could be second argument.
In theory, the formula already can specify the assay.type = NULL, row.var = NULL, col.var = NULL,. They could be hidden options to overwrite formula when left-hand-side matching is not explicit (e.g., matching with colData and assay)
formula = counts ~ disease
or
formula ~ shannon ~ disease
There was a problem hiding this comment.
split.by could be also hidden option. Usually it is not recommended to use.
2 participants
No description provided.