easystats / bayestestR

:ghost: Utilities for analyzing Bayesian models and posterior distributions
https://easystats.github.io/bayestestR/
GNU General Public License v3.0
563 stars 55 forks source link

Rejuvenation #625

Closed strengejacke closed 1 year ago

strengejacke commented 1 year ago

Fixes #525

Fixing workflows, possibly unresolved issues from #618?

Related to:

strengejacke commented 1 year ago

@IndrajeetPatil Here are some issues with not available packages, do you know how to fix this? https://github.com/easystats/bayestestR/actions/runs/6361179508/job/17275434553?pr=625

IndrajeetPatil commented 1 year ago

@IndrajeetPatil Here are some issues with not available packages, do you know how to fix this? https://github.com/easystats/bayestestR/actions/runs/6361179508/job/17275434553?pr=625

Should be fixed for runs from here on.

strengejacke commented 1 year ago

Vignette probability of direction is broken. Seems like this code no longer works:

library(ggplot2)
library(bayestestR)
library(logspline)
library(KernSmooth)
#> KernSmooth 2.23 loaded
#> Copyright M. P. Wand 1997-2009

# Compute the correlations
data <- data.frame()
for (the_mean in runif(25, 0, 4)) {
  for (the_sd in runif(25, 0.5, 4)) {
    x <- rnorm(100, the_mean, abs(the_sd))
    data <- rbind(
      data,
      data.frame(
        direct = as.numeric(pd(x)),
        kernel = as.numeric(pd(x, method = "kernel")),
        logspline = as.numeric(pd(x, method = "logspline")),
        KernSmooth = as.numeric(pd(x, method = "KernSmooth"))
      )
    )
  }
}
data <- as.data.frame(sapply(data, as.numeric))

# Visualize the correlations
bayesplot::mcmc_pairs(data) +
  theme_classic()
#> Warning: Only one chain in 'x'. This plot is more useful with multiple chains.
#> NULL

Created on 2023-09-30 with reprex v2.0.2

strengejacke commented 1 year ago

I think we also need updated/new methods for get_predicted(), see failing tests here: https://github.com/easystats/bayestestR/actions/runs/6361855591/job/17276543814?pr=625#step:9:348

strengejacke commented 1 year ago

Ok, I think we're coming close to get everything right again. One thing I'm not sure: the .get_predicted method, should they return a single value, like

#' @export
p_significance.get_predicted <- function(x, threshold = "default", ...) {
  p_significance(as.numeric(x), threshold = threshold, ...)
}

or should they be applied to all iterations, like:

#' @export
spi.get_predicted <- function(x, ...) {
  if ("iterations" %in% names(attributes(x))) {
    out <- spi(as.data.frame(t(attributes(x)$iterations)), ...)
  } else {
    insight::format_error("No iterations present in the output.")
  }
  attr(out, "object_name") <- insight::safe_deparse_symbol(substitute(x))
  out
}
DominiqueMakowski commented 1 year ago

should they return a single value

I'd say a single value, because the font-fracing output of get_predicted is a vector, it's just because of the as.data.frame() that pulls all the draws that we ended up with some downstream unexpected changes here.

but in any case computing this stuff on the output of get_predicted is a bit odd

strengejacke commented 1 year ago

Hm, there were actually quite some bugs in package functions which were not captured by the CI tests? @mattansb Here's an example that seems to have worked in the past (at least, we had a test for it), but now no longer works:

library(bayestestR)
library(blavaan)
#> Loading required package: Rcpp
#> This is blavaan 0.5-1
#> On multicore systems, we suggest use of future::plan("multicore") or
#>   future::plan("multisession") for faster post-MCMC computations.

data("PoliticalDemocracy", package = "lavaan")

model <- "
  # latent variable definitions
  dem60 =~ y1 + a*y2
  dem65 =~ y5 + a*y6

  # regressions
  dem65 ~ dem60

  # residual correlations
  y1 ~~ y5
"

model2 <- "
  # latent variable definitions
  dem60 =~ y1 + a*y2
  dem65 =~ y5 + a*y6

  # regressions
  dem65 ~ 0*dem60

  # residual correlations
  y1 ~~ 0*y5
"
suppressWarnings(capture.output({
  bfit <- blavaan::bsem(model,
    data = PoliticalDemocracy,
    n.chains = 1, burnin = 50, sample = 100
  )
  bfit2 <- blavaan::bsem(model2,
    data = PoliticalDemocracy,
    n.chains = 1, burnin = 50, sample = 100
  )
}))

bayesfactor_models(bfit, bfit2)
#> Warning: Bayes factors might not be precise.
#>   For precise Bayes factors, sampling at least 40,000 posterior samples is
#>   recommended.
#> Error in apply(diffs, 2, sum): dim(X) must have a positive length

Created on 2023-10-02 with reprex v2.0.2

codecov[bot] commented 1 year ago

Codecov Report

Merging #625 (bb91bfb) into main (5f81276) will increase coverage by 2.36%. Report is 31 commits behind head on main. The diff coverage is 69.48%.

:exclamation: Current head bb91bfb differs from pull request most recent head 36f0a3e. Consider uploading reports for the commit 36f0a3e to get more accurate results

@@            Coverage Diff             @@
##             main     #625      +/-   ##
==========================================
+ Coverage   50.87%   53.24%   +2.36%     
==========================================
  Files          65       65              
  Lines        5185     5287     +102     
==========================================
+ Hits         2638     2815     +177     
+ Misses       2547     2472      -75     
Files Coverage Δ
R/area_under_curve.R 77.77% <ø> (ø)
R/bayesfactor_inclusion.R 96.55% <ø> (ø)
R/bayesfactor_models.R 75.94% <ø> (ø)
R/bayesfactor_restricted.R 66.66% <ø> (ø)
R/check_prior.R 0.00% <ø> (ø)
R/contr.equalprior.R 82.60% <100.00%> (+0.79%) :arrow_up:
R/convert_bayesian_to_frequentist.R 46.25% <ø> (ø)
R/convert_pd_to_p.R 80.00% <100.00%> (+80.00%) :arrow_up:
R/describe_prior.R 70.83% <ø> (ø)
R/effective_sample.R 50.00% <ø> (ø)
... and 31 more

... and 3 files with indirect coverage changes

IndrajeetPatil commented 1 year ago

Thanks, @strengejacke!

strengejacke commented 1 year ago

We should schedule a new release-round... Something like

easystats -> see -> insight -> bayestestR -> ... (parameters? performance?)

strengejacke commented 1 year ago

what about report, @rempsyc?

datawizard, parameters and performance are quite recent, so probably no update needed shortly.

DominiqueMakowski commented 1 year ago

Shouldn't easystats be last to have new versions reqs?

strengejacke commented 1 year ago

Yes, makes sense.

rempsyc commented 12 months ago

It's been almost 28 weeks for report, I'm happy to resubmit to CRAN. Seems like the other easystats packages devel versions I was relying on (insight, effectsize) were already pushed to CRAN, so technically I could do it next weekend?

strengejacke commented 12 months ago

I suggest adding all GitHub version to the REMOTES field, so you can test report on the latest devel versions first - otherwise, we may find revdep issues when we submit packages after report was accepted on CRAN.

rempsyc commented 12 months ago

Oh good idea, thanks!