frankportman/bayesab

🐢 bayesAB: Fast Bayesian Methods for A/B Testing

bayesab's Issues

Issues with next ggplot2 release

Hi

We are working on the next ggplot2 release and bayesAB fails its unit tests due to a new deprecation warning in ggplot2 which breaks the silent expectations in

bayesAB/tests/testthat/test-generics.R

Lines 39 to 63 in 9db6f46

	test_that("Success", {
	expect_silent(plot(x))
	expect_silent(plot(x, rep(.5, 4)))
	expect_equal(length(plot(x, posteriors = FALSE, samples = FALSE)$priors), 1)
	expect_equal(names(plot(x, posteriors = FALSE, samples = FALSE)$priors), c('NormalInvGamma'))
	expect_silent(print(plot(x)))
	expect_silent(print(plot(x, rep(.5, 4))))
	expect_output(print(x), "Distribution used")
	expect_silent(summary(x))
	expect_silent(summary(x, rep(.5, 4)))
	expect_silent(summary(x, rep(.5, 4), rep(.5, 4)))
	expect_output(print(summary(x)), 'P(A > B)', fixed = TRUE)
	expect_output(print(summary(AB3)), 'P(A > B)', fixed = TRUE)
	expect_is(c(x, x), "bayesTest")
	expect_identical(rep(x$posteriors$Mu$A, 2), c(x, x)$posteriors$Mu$A)
	expect_identical(rep(x$posteriors$Mu$B, 2), c(x, x)$posteriors$Mu$B)
	expect_identical(rep(x$posteriors$Mu$A, 3), c(x, x, x)$posteriors$Mu$A)
	})

We plan to submit on May 31st and hope you are able to fix these small issues beforehand

Add support for arbitrary # of 'tests'?

This is including one variable, where all you want is to sample from the posterior and get a parameter estimate. Is this outside the scope of the package / does it break the current purpose?

README images

Open up summary function API

Run Simulations in Parallel

Is it possible to run BayesTest in parallel?

Looking for way to run 1M permutations faster.

Loss calculation in getPostError

I think to calculate the expected loss for choosing A over B, in this case, is to calculate the percentage change of A over B, when B is better than A. Should the formula be: loss = -f(A_samples, B_samples) instead of loss = f(B_samples, A_samples)?

bayesAB/R/util.R

Line 19 in 9db6f46

loss <- f(B_samples, A_samples)

Give meaningful titles to `summary` section in `print.bayesTest` for `combine`d tests

It's currently labeled A_data and B_data, until there are more than 2 columns, then it breaks. Need an elegant way of doing this.

Add "batteries included" shiny app

Get Posterior Error Function

Can you explain how does this function work? When I try running it on two dummy vectors, I get an Inf.
Do A_samples and B_samples encode the entire data vectors? Is this the squared loss? Can you explain the logic behind this implementation? What does loss do if A_samples and B_samples have 0s?
I highlighted the unclear code snipets.
`set.seed(1)
a <- rbinom(100, 1, .5)
b <- rbinom(100, 1, .6)
coalesce <- function(n) ifelse(is.na(n) | is.nan(n), 0, n)

Expected loss from switching from B to A

getPostError <- function(A_samples, B_samples, f = function(a, b) (a-b)/b) {
BoverA <- B_samples > A_samples
loss <- f(B_samples, A_samples)
coalesce(mean(BoverA) * mean(loss[BoverA]))
}
getPostError(b,a)`

How does summary(bayesTest(a,b, priors = c('alpha' = 1, 'beta' = 1), n_samples = 1e5, distribution = 'bernoulli')) reach a posterior loss of 0.260855 ?

Thank you very much for making this clear.

Plot bayesTest object or plot summary object?

Both summary and plot generics take 'percentLift' as an argument. Does it make more sense for the API to plot the summary object or the base bayesTest object? @JesseKolb

Forthcoming ggplot2 release and bayesAB

We're in the process of preparing a ggplot2 release. As part of the release process, we run the R CMD check on packages that use ggplot2 to make sure we don't accidentally break code for downstream packages.

In running the R CMD check on bayesAB, we identified the following issue:

• checking examples ... ERROR

  ...
  $Probability
  [1] 0.04541
  
  --------------------------------------------
  
  Credible Interval on (A - B) / B for interval length(s) (0.9) : 
  
  $Probability
            5%          95% 
  -0.355943366 -0.006198834 
  
  --------------------------------------------
  
  Posterior Expected Loss for choosing B over A:
  
  $Probability
  [1] 0.2601664
  
  > plot(AB1)
  Error: Either ymin or ymax must be given as an aesthetic.
  Execution halted
  

These failures are because bayesAB is using geom_ribbon() without layer data. The behaviour of layers when "setting" an aesthetic (outside aesI()) with length > 1 is not defined and may change in the future (in the case of geom_ribbon(), we now require that at least one of ymin or ymax is mapped to prevent bugs from occurring).

To fix this error, we suggest using a geom_*() function with its own data. Note that using .data$col_name within aes() is the preferred way to avoid CMD check issues about undefined variables when mapping columns (make sure you include #' importFrom rlang .data in at least one roxygen documentation block to avoid an error about the name .data being undefined).

ggplot2::ggplot() +
  ggplot2::geom_ribbon(
    ggplot2::aes(x = .data$x, ymax = .data$ymax),
    data = data.frame(x = 1:5, ymax = c(0, 1, 3, 1, 0)),
    ymin = 0
  )

^{Created on 2019-05-09 by the reprex package (v0.2.1)}

We hope to release the new version of ggplot2 in the next two weeks, at which point you will get a note from CRAN that your package checks are failing. Let me know if I can help!

Open up PEL API

Elegant way of handling *both* raw data (A_data, B_data) and summarized data (A_counts, A_exposure, etc...)

Next release I want to have both the current setup (input A_data, B_data directly) and summarized data (A_sum, A_length, B_sum, B_length). This should not complicate the architecture if done correctly.

Issues with next version of ggplot2

Hi

We preparing the next release of ggplot2 and our reverse dependency checks show that your package is failing with the new version. Looking into it we see that this is due to changes in the warnings and errors thrown by ggplot2 that you test for in your package

You can install the release candidate of ggplot2 using devtools::install_github('tidyverse/[email protected]') to test this out.

We plan to submit ggplot2 by the end of October and hope you can have a fix ready before then

Kind regards
Thomas

Update rule in drawMusAndSigmas of dist-normal.R seems incorrect

Your code does not correspond to the update rule in https://en.wikipedia.org/wiki/Conjugate_prior, which is the same as https://www.cs.ubc.ca/~murphyk/Papers/bayesGauss.pdf under a different parameterization.

Another problem: when sampling from the gamma distribution, the default arguments have rate as the third argument instead of shape. You would need to name your shape argument.

Great package overall. Thanks for the great work.

Should percentLift/credInt args to generics be named?

I think these args should be named lists/vectors in order to not be ambiguous.

Alternatively, we can make them constant (scalar) for a single invocation of plot/summary. However this is less flexible.

Support for more than 2 groups

Hi!
Since a lot of AB tests involve more than 2 groups, have you thought about adding support for more 2 groups? Or do you advise using the current structure and just compare all groups to A?

Thank you for creating the package.

Best,
Miha

Add documentation about percent change insignificance for negative numbers

Posterior Expected Loss backwards?

I believe

bayesAB/R/generics.R

Line 188 in 515a139

cat('Posterior Expected Loss for choosing B over A:\n\n')

should be:

Posterior Expected Loss for choosing A over B

If I'm not mistaken, the number comes from:

bayesAB/R/util.R

Line 16 in 515a139

# Expected loss from switching from B to A

Do I have that right? If so I can submit the one-line PR.