-
Notifications
You must be signed in to change notification settings - Fork 27
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Rename cluster to addCluster #502
Conversation
fix identation
Merge branch 'master' of https://github.com/microbiome/mia into rename_cluster # Conflicts: # R/deprecate.R # man/deprecate.Rd
Merge branch 'master' of https://github.com/microbiome/mia into rename_cluster # Conflicts: # R/cluster.R # tests/testthat/test-9cluster.R
Merge branch 'master' of https://github.com/microbiome/mia into rename_cluster # Conflicts: # NEWS
R/deprecate.R
Outdated
#' @param x A | ||
#' \code{\link[SummarizedExperiment:SummarizedExperiment-class]{SummarizedExperiment}} | ||
#' object. | ||
#' | ||
#' @param clust.col A single character value indicating the name of the | ||
#' \code{rowData} (or \code{colData}) where the data will be stored. | ||
#' | ||
#' @param exprs_values a single \code{character} value for specifying which | ||
#' assay to use for calculation. | ||
#' (Please use \code{assay.type} instead.) | ||
#' | ||
#' @param transposed Logical scalar, is x transposed with cells in rows? | ||
#' | ||
#' @param type the type of measure used for the goodness of fit. One of |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This manual page should collect all deprecated functions. Some of them might have same parameter name but different meaning. So I'm wondering what happens then. --> I'm wondering if it is better to not list parameters for any of these functions since user's should not use them anyways (even though BIocCheck would tell otherwise)
R/deprecate.R
Outdated
name = "clusters", clust.col = "clusters", ...){ | ||
.Deprecated(msg = paste0("'cluster' is deprecated. ", | ||
"Use 'addCluster' instead.")) | ||
addCluster(x) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This does not pass parameters. addCluster gets only x parameter. Not others. Moreover, you can skip listing parameters. Use ... and then pass them to addCluster with ...
Only x is needed to list since the method is chosen based on class of x
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The idea is to keep these functions a hollow as possible. Just that they work but do not do anything but call the new function
R/calculateDMM.R
Outdated
setMethod("calculateDMN", signature = c(x = "SummarizedExperiment"), | ||
function(x, assay.type = assay_name, assay_name = exprs_values, exprs_values = "counts", | ||
transposed = FALSE, ...){ | ||
.Deprecated(old="calculateDMN", new="cluster", | ||
"Now calculateDMN is deprecated. Use cluster with DMMParam parameter instead.") | ||
mat <- assay(x, assay.type) | ||
if(!transposed){ | ||
mat <- t(mat) | ||
} | ||
calculateDMN(mat, ...) | ||
} | ||
function(x, assay.type = assay_name, assay_name = exprs_values, exprs_values = "counts", | ||
transposed = FALSE, ...){ | ||
.Deprecated(old="calculateDMN", new="cluster", | ||
"Now calculateDMN is deprecated. Use cluster with DMMParam parameter instead.") | ||
mat <- assay(x, assay.type) | ||
if(!transposed){ | ||
mat <- t(mat) | ||
} | ||
calculateDMN(mat, ...) | ||
} | ||
) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems that the indentation is incorrect. Also lines are too long (80 characters is maximum)
Can you either fix these or revert the file? Maybe remove the changes, since this PR is about cluster function
Otherwise, this is ready to go. Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! @antagomir
There are unresolved comments, and one conflict. If those are resolved then this seems good to me at least? |
Merge branch 'devel' of https://github.com/microbiome/mia into rename_cluster # Conflicts: # NEWS
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## devel #502 +/- ##
==========================================
- Coverage 73.11% 72.90% -0.21%
==========================================
Files 40 40
Lines 4630 4643 +13
==========================================
Hits 3385 3385
- Misses 1245 1258 +13 ☔ View full report in Codecov by Sentry. |
Rename
cluster
toaddCluster.
Fix indentation in calculateDMM and cluster files following Bioconductor best practices (indentation must be a multiple of 4 spaces).
Move deprecated functions from the calculateDMM file to deprecate file.