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

Adding ab-testing vignette #409

Merged
merged 8 commits into from
Oct 15, 2020
Merged

Adding ab-testing vignette #409

merged 8 commits into from
Oct 15, 2020

Conversation

imadmali
Copy link
Collaborator

@imadmali imadmali commented Jan 4, 2020

I'd like to contribute a vignette that outlines how to do A/B testing inference (post-experiment) with rstanarm functions. As discussed here I think it would be useful if it was part of the "Additional tutorials on specific modeling techniques and applications" section on the vignette page of the rstanarm site. @jgabry I've left your comments in and have changed the content based on some of them. Also, regarding Appendix B, I don't fully understand how priors are specified on group specific intercepts when using stan_glmer so I just said something to the effect of "default priors are used".

Copy link
Member

@jgabry jgabry left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a few minor comments (I see you already addressed previous comments, thanks!). For appendix B, are you ok leaving it just saying we're using default priors or do you want to add the details about the specification? It's a bit more complicated than the other priors.

vignettes/ab-testing.Rmd Outdated Show resolved Hide resolved
vignettes/ab-testing.Rmd Outdated Show resolved Hide resolved
vignettes/ab-testing.Rmd Show resolved Hide resolved
vignettes/ab-testing.Rmd Outdated Show resolved Hide resolved
knitr::opts_chunk$set(collapse = TRUE)
library(rstan)
library(rstanarm)
library(dplyr)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible to do this without dplyr? I don't think we currently have dplyr as a dependency but we would need to add it to use it in the vignette.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So I use dplyr here. I don't have to use it and it's straightforward to rewrite this in non-dplyr code. But since it's pretty much a standard for industry workflow maybe it's something to consider using in all the vignettes. But then, like you mentioned, it's not a dependency for the main purpose of the package. Let me know what you want me to do.

@imadmali
Copy link
Collaborator Author

imadmali commented Mar 2, 2020

@jonah regarding the hierarchical section I'm fine with saying default priors if there is a place where the information about the priors is clearly defined? If there is such a place then I'll provide a link to it. Let me know.

@imadmali
Copy link
Collaborator Author

imadmali commented Jul 5, 2020

@jgabry I made some more edits. Let me know if it's okay to merge or if more changes need to be made.

html_vignette:
toc: yes
params:
EVAL: !r identical(Sys.getenv("NOT_CRAN"), "true")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can't use the params mechanism any more because some OS does not have a sufficiently recent Pandoc or something. The way we are doing the other vignettes now is to put knitr::opts_chunk$set(eval = identical(Sys.getenv("NOT_CRAN"), "true")))

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I got around this in other packages just by checking if (exists("params")) but not using it is also fine.

Copy link
Collaborator Author

@imadmali imadmali Aug 14, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I'll just remove it then and add that line of code.

@ssp3nc3r
Copy link

ssp3nc3r commented Aug 7, 2020

Nice vignette. Would it make sense to call set.seed() for repeated calls to posterior_predict when comparing posteriors, or just run the function once and subtract the groups? For example,

pp_c <- posterior_predict(fit_group_bin)[,1]
pp_d <- posterior_predict(fit_group_bin)[,2]

would be

pp <- posterior_predict(fit_group_bin)
pp_c <- pp[,1]
pp_d <- pp[,2]

@jgabry
Copy link
Member

jgabry commented Aug 8, 2020

@ssp3nc3r Yeah good suggestion.

@imadmali
Copy link
Collaborator Author

I've made all suggested edits. Let me know if I'm missing anything.

@imadmali
Copy link
Collaborator Author

@jgabry @bgoodri is there anything left that I need to do to get this accepted?

Copy link
Member

@jgabry jgabry left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@imadmali Sorry for the delay. I think this is ready to go, just one quick change: can you remove the uses of autoscale=FALSE? With the latest release that's no longer necessary when specifying non-default priors. It's not wrong, it's just not necessary anymore so might be confusing to keep it.

Copy link
Member

@jgabry jgabry left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just went through and removed autoscale=FALSE since it's not needed anymore with the latest release. AFAIK that was the last remaining thing to change so will merge this (can revert if necessary).

@jgabry jgabry merged commit 272a04d into master Oct 15, 2020
@jgabry jgabry deleted the doc/ab-testing branch October 15, 2020 23:28
jgabry added a commit that referenced this pull request Oct 15, 2020
add news items for #409, #415, and #459
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants