-
Notifications
You must be signed in to change notification settings - Fork 56
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
as_matrix doesn't return the exact diagonal of a cordf object #52
Comments
Excellent point. My original idea was to convert back state that |
Yeah, I think the correct behaviour would be something like: inherit the object as is but replace the diagonal if it's specified. But considering that So something like as_matrix <- function(x, diagonal = NA) {
UseMethod("as_matrix")
}
as_matrix.cor_df <- function(x, diagonal = NA) {
# Separate rownames
row_names <- x$rowname
x %<>% dplyr::select_("-rowname")
# Replace diagonal if specified
if (!is.na(diagonal)) diag(x) <- diagonal
# Convert to matrix and set rownames
class(x) <- "data.frame"
#x %<>% as.matrix()
x <- as.matrix(x)
rownames(x) <- row_names
x
} If that makes sense, I can wrap it up in a pull request, also including the minor backward incompatible change in the |
Why replace the diagonal if specified it? E.g., if I run In this case, could leave the argument as optional: as_matrix.cor_df <- function(x, diagonal) {
...
if (!missing(diagonal)) diag(x) <- diagonal
...
} Thoughts? Aside, if this does result in backward incompatible change, (1) great idea to add to |
I think in principle both approaches are similar, no? If it is non-NA or if it's non-missing, replace the diagonal. The only difference is the default argument. In any case, I think it's the way to go, because it gives the option to replace the diagonal once again. Ok, if this is alright I'll wrap your suggestions with the |
Yep, go for it. I think you should put yourself as a contributor in the |
See #55 |
This issue has been automatically locked. If you believe you have found a related problem, please file a new issue (with a reprex: https://reprex.tidyverse.org) and link to this issue. |
With the development version of
corrr
I understand that
as_matrix
has adiagonal
argument but shouldn't it return the samecor_df
? Let me know if this makes sense and I'll work the out the code and send a PR.The text was updated successfully, but these errors were encountered: