as_matrix doesn't return the exact diagonal of a cordf object

[cimentadaj]

With the development version of corrr

library(corrr)
#> Loading required package: dplyr
#> 
#> Attaching package: 'dplyr'
#> The following objects are masked from 'package:stats':
#> 
#>     filter, lag
#> The following objects are masked from 'package:base':
#> 
#>     intersect, setdiff, setequal, union

x <- correlate(mtcars, diagonal = 2)
#> 
#> Correlation method: 'pearson'
#> Missing treated using: 'pairwise.complete.obs'
diag(as_matrix(x))
#>  mpg  cyl disp   hp drat   wt qsec   vs   am gear carb 
#>    1    1    1    1    1    1    1    1    1    1    1

I understand that as_matrix has a diagonal argument but shouldn't it return the same cor_df? Let me know if this makes sense and I'll work the out the code and send a PR.

[drsimonj]

Excellent point. My original idea was to convert back state that cor() would have created. Perhaps could leave diagonal as an optional argument. Default to NULL, in which case no change is made. Thoughts?

[cimentadaj]

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 as_matrix already inherits the params from as_cordf, to avoid inconsistencies, we can set diagonal to NA as it is already the default value in as_cordf.

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 NEWS.md file.

[drsimonj]

Why replace the diagonal if specified it? E.g., if I run correlate(x, diagonal = 100), wouldn't I want to preserve the 100 diagonal with as_matrix()?

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 NEWS.md; (2) we should consider version number jump to 0.3.0 for next CRAN submission.

[cimentadaj]

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 NEWS.md and the version bump into a pull request this week.

[drsimonj]

Yep, go for it. I think you should put yourself as a contributor in the DESCRIPTION file too!

[drsimonj]

See #55

[github-actions]

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.