Skip to content
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

Bayesian correlations not working with the latest version of {parameters} #269

Closed
IndrajeetPatil opened this issue Oct 7, 2022 · 12 comments · Fixed by #270
Closed

Bayesian correlations not working with the latest version of {parameters} #269

IndrajeetPatil opened this issue Oct 7, 2022 · 12 comments · Fixed by #270
Assignees
Labels
bug 🐛 Something isn't working high priority 🏃‍♂️ This issue should be addressed soon

Comments

@IndrajeetPatil
Copy link
Member

{ggstatsplot} is broken because of this issue in {correlation}.

I can't track down what change in the ecosystem led to the change in behaviour of the CRAN-version of correlation, because the following used to work:

library(correlation)
library(dplyr, warn.conflicts = FALSE)
df <- ggplot2::msleep

correlation(filter(df, vore == "carni"), bayesian = TRUE)
#> Warning in genhypergeo_series_pos(U = c((n - 1)/2, (n - 1)/2), L = ((n + :
#> Series not converged.
#> Error in rbind(deparse.level, ...): numbers of columns of arguments do not match

Created on 2022-10-07 with reprex v2.0.2

@IndrajeetPatil IndrajeetPatil added bug 🐛 Something isn't working high priority 🏃‍♂️ This issue should be addressed soon labels Oct 7, 2022
@DominiqueMakowski
Copy link
Member

Warning in genhypergeo_series_pos

I started having this warning fairly often too for bayesian correlations (but no errors in correlation), did something change in BayesFactor to cause that?

@IndrajeetPatil
Copy link
Member Author

I doubt it, since BayesFactor was last updated in July, and we would have noticed such a change way earlier instead of now.

I have a hunch that this has to do with a change in either parameters or bayestestR.

@strengejacke
Copy link
Member

Here's a reprex:

library(parameters)
library(dplyr, warn.conflicts = FALSE)
df <- as.data.frame(ggplot2::msleep)

x <- filter(df, vore == "carni")
rez <- BayesFactor::correlationBF(x$sleep_total, x$awake, rscale = "medium")
#> Warning in genhypergeo_series_pos(U = c((n - 1)/2, (n - 1)/2), L = ((n + :
#> Series not converged.
rez
#> Bayes factor analysis
#> --------------
#> [1] Alt., r=0.333 : NA ±0%
#> 
#> Against denominator:
#>   Null, rho = 0 
#> ---
#> Bayes factor type: BFcorrelation, Jeffreys-beta*

params <- parameters::model_parameters(rez)
params
#> Bayesian correlation analysis
#> 
#> Parameter | Median |         95% CI |   pd |         Prior
#> ----------------------------------------------------------
#> rho       |  -1.00 | [-1.00, -1.00] | 100% | Beta (3 +- 3)
#> 
#> Uncertainty intervals (equal-tailed) and p-values (two-tailed) computed
#>   using a MCMC distribution approximation.

Created on 2022-10-07 with reprex v2.0.2

@DominiqueMakowski
Copy link
Member

could it be with the removal of ROPE?

@strengejacke
Copy link
Member

strengejacke commented Oct 7, 2022

parameters uses datawizard::remove_empty(), which removes columns with completely NA (which is a new behaviour, I think), so the BF is missing for this particular combination of variables, which causes the error in correlation.

A fix would be to check here:

params <- parameters::model_parameters(

if the BF column exists, and if not, add BF <- NA. We must ensure the columns have the right order (so we might need to sort), so params can be rbind() to the final results (see cor_test()).

@strengejacke
Copy link
Member

strengejacke commented Oct 7, 2022

We should add @IndrajeetPatil example as test, once the issue is resolved.

@DominiqueMakowski
Copy link
Member

DominiqueMakowski commented Oct 7, 2022

shouldnt we add that in parameters? The output should include the BF col even if NA

@strengejacke
Copy link
Member

Yes, I agree. But maybe for now it's better to have this fix in correlation, too? I just submitted parameters two days ago. 😬

@DominiqueMakowski
Copy link
Member

haha okay makes sense then

@strengejacke
Copy link
Member

There must have been a reason why we changed the behaviour in model_parameters.BFBayesFactor(). We added

  # ==== remove rows and columns with complete `NA`s
  out <- datawizard::remove_empty(out)

I think it's related to the new API of effectsize, which might return empty columns or rows that we didn't wanted to have? But we were not considering that the BF could be NA.

strengejacke added a commit to easystats/parameters that referenced this issue Oct 7, 2022
@strengejacke
Copy link
Member

Good that I have copied the clean branch after CRAN submission of parameters (https://github.com/easystats/parameters/tree/release_0_19_0_branch) - the main-branch now includes several breaking changes that are not properly tested and might break other packages, so it's possible to include the patch into the release_0_19_0_branch and submit a fix to CRAN, if necessary. :-)

@IndrajeetPatil
Copy link
Member Author

I think it's related to the new API of effectsize, which might return empty columns or rows that we didn't wanted to have? But we were not considering that the BF could be NA.

Yes, the change in parameters is good. We just need to make a few exceptions, where necessary.

I have added tests and done some other preparations necessary for a new CRAN release of correlation.

Are there any other high prio issues that need to be resolved before this happens?

cc @bwiernik

strengejacke added a commit to easystats/parameters that referenced this issue Oct 7, 2022
@IndrajeetPatil IndrajeetPatil changed the title {correlation} bug is causing {ggstatsplot} check failures Bayesian correlations not working with the latest version of {parameters} Oct 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 Something isn't working high priority 🏃‍♂️ This issue should be addressed soon
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants