Bayesian ab testing for PyMC v4 #351
Conversation
|
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
|
View / edit / reply to this conversation on ReviewNB aloctavodia commented on 2022-06-01T12:49:35Z Why setting return_inferencedata=False, instead of using the default? drbenvincent commented on 2022-06-01T13:27:23Z Yep - ideally we do return idata objects (which is default behaviour). But that might require some updating of post-sampling code such as the plotting |
|
View / edit / reply to this conversation on ReviewNB aloctavodia commented on 2022-06-01T12:49:36Z idem previous comment |
|
View / edit / reply to this conversation on ReviewNB aloctavodia commented on 2022-06-01T12:49:37Z Line #7. axs[1].axvline(x=0, color="red");
|
|
View / edit / reply to this conversation on ReviewNB aloctavodia commented on 2022-06-01T12:49:38Z Line #11. trace_weak = pm.sample(draws=5000, cores=1, chains=2) better to set Do we need to set |
|
View / edit / reply to this conversation on ReviewNB aloctavodia commented on 2022-06-01T12:49:39Z Line #11. trace = pm.sample(draws=5000, chains=2, cores=1) better set percevalve commented on 2022-06-01T14:49:22Z Just going to remove all kwargs in this notebook, my understanding of the NB is that this not critical. |
|
View / edit / reply to this conversation on ReviewNB aloctavodia commented on 2022-06-01T12:49:39Z can we use the default? |
|
View / edit / reply to this conversation on ReviewNB aloctavodia commented on 2022-06-01T12:49:40Z Line #4. ax.axvline(x=0, color="red");
|
|
View / edit / reply to this conversation on ReviewNB aloctavodia commented on 2022-06-01T12:49:41Z Line #17. trace = pm.sample(draws=5000, chains=2, cores=1) idem previous comments |
|
View / edit / reply to this conversation on ReviewNB aloctavodia commented on 2022-06-01T12:49:42Z I think this message disapear if you run the last version from main, not sure |
|
Thanks @percevalve for your help. I leave a few comments. |
|
View / edit / reply to this conversation on ReviewNB drbenvincent commented on 2022-06-01T12:57:29Z Line #9. import pymc.math as pmm I think importing like this is not standard practice. Maybe remove this line, and replace |
|
View / edit / reply to this conversation on ReviewNB drbenvincent commented on 2022-06-01T12:57:30Z Maybe remove all chains and cores kwargs for this notebook. Unless there is some reason why they are essential that I'm not aware of. percevalve commented on 2022-06-01T14:47:16Z Does not look like it, will just remove them. |
|
View / edit / reply to this conversation on ReviewNB drbenvincent commented on 2022-06-01T12:57:31Z The x-axis scaling behaviour of these plots has changed compared to the previous version. The previous version was clearer as the range was zoomed in to where the data is. Not sure what has caused that but ideally we'd be able to zoom to the data range of the x-axis. percevalve commented on 2022-06-01T15:55:39Z Looks like there was a change somewhere, but cannot find where, setting a bin number seems to do the trick. Goign to add a variable to ensure consistency across all plot_posterior graphs. |
|
Yep - ideally we do return idata objects (which is default behaviour). But that might require some updating of post-sampling code such as the plotting View entire conversation on ReviewNB |
|
OK then, so we can keep the return_inference_data = false for now and open an issue so we can later change it. |
|
Does not look like it, will just remove them. View entire conversation on ReviewNB |
|
Just going to remove all kwargs in this notebook, my understanding of the NB is that this not critical. View entire conversation on ReviewNB |
|
Looks like there was a change somewhere, but cannot find where, setting a bin number seems to do the trick. Goign to add a variable to ensure consistency across all plot_posterior graphs. View entire conversation on ReviewNB |
percevalve
force-pushed
the
bayesian_ab_testing
branch
2 times, most recently
from
1b4f550
to
851e554
Compare
percevalve
force-pushed
the
bayesian_ab_testing
branch
from
851e554
to
b11a5b5
Compare
|
View / edit / reply to this conversation on ReviewNB drbenvincent commented on 2022-06-01T17:25:57Z Line #12. print(f"Running on PyMC v{pm.__version__}")
We can remove this line. The watermark stuff at the bottom of the notebooks provides all the version info we need, |
|
View / edit / reply to this conversation on ReviewNB percevalve commented on 2022-06-01T19:16:36Z Line #11. ) The hist plots do no properly scale if number of bins is not specified. 2 solutions seems to work:
To keep consistency for all graphs, added a aloctavodia commented on 2022-06-01T20:53:04Z in ArviZ histograms are assumed by default to represent discrete variables, and the numbers of bins take that into account. So for continuous variables or when the number of discrete variables is very large they may require to change how bins are computed, for example by passing a number of bins |
|
in ArviZ histograms are assumed by default to represent discrete variables, and the numbers of bins take that into account. So for continuous variables or when the number of discrete variables is very large they may require to change how bins are computed, for example by passing a number of bins View entire conversation on ReviewNB |
Update to enable running under PyMC v4
Addresses Issue #172
return_inferencedata=Falseas part of the code assumespm.backends.base.MultiTraceformatting.pm.backends.base.MultiTraceandaz.InferenceData