glm bambi usage (WIP) #177
Conversation
|
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
|
Skimmed this over and it looks great to me. Will do a more in depth review as well |
|
View / edit / reply to this conversation on ReviewNB canyon289 commented on 2021-06-12T03:58:05Z This is a nitpick. I think this notebook would benefit from the model graphs that pymc3 can generate using the psuedo code below.
But before you add it lets see if anyone else agrees, otherwise feel free to ignore this
pooled_sales_diagram = pm.model_to_graphviz(model.the_pymc_model_i_forgot_how_to_get_it)
tomicapretto commented on 2021-06-12T12:36:50Z You can do
It calls OriolAbril commented on 2021-06-12T13:45:47Z also note the plot is now plotting x and y instead of x_out and y_out which are the real observations
|
|
You can do
It calls View entire conversation on ReviewNB |
|
also note the plot is now plotting x and y instead of x_out and y_out which are the real observations
View entire conversation on ReviewNB |
|
View / edit / reply to this conversation on ReviewNB OriolAbril commented on 2021-06-12T13:59:24Z here bambi is used for the irst time and therefore introduced to readers we have to assume. There should be a markdown cell right before with a couple sentencess and a link to bambi tomicapretto commented on 2021-06-14T00:19:56Z Agree on this. Something inspired on our overview here https://github.com/bambinos/bambi and one sentence saying it uses a formula for the model is enough I think. tomicapretto commented on 2021-06-14T00:21:50Z I would also mention priors are automatically selected when the user does not specify them. Something in the same spirit than the block that used to start with "Since there are a couple of general linear models that are being" |
|
View / edit / reply to this conversation on ReviewNB OriolAbril commented on 2021-06-12T13:59:25Z the dict called data should not be recomputed, it was intended to show how with the same data but adding 3 outliers we have to change the model |
|
View / edit / reply to this conversation on ReviewNB OriolAbril commented on 2021-06-12T13:59:25Z much of this explanation is no longer relevant |
|
View / edit / reply to this conversation on ReviewNB OriolAbril commented on 2021-06-12T13:59:26Z this needs to be updated to work on windows: https://github.com/pymc-devs/pymc3/wiki/PyMC3-Jupyter-Notebook-Style-Guide |
|
View / edit / reply to this conversation on ReviewNB OriolAbril commented on 2021-06-12T13:59:26Z this needs a link to bambi or formulae to explain syntax of group effects |
|
View / edit / reply to this conversation on ReviewNB OriolAbril commented on 2021-06-12T13:59:27Z might want to set |
|
View / edit / reply to this conversation on ReviewNB OriolAbril commented on 2021-06-12T13:59:28Z reading the dta here should follow https://github.com/pymc-devs/pymc3/wiki/PyMC3-Jupyter-Notebook-Style-Guide |
|
View / edit / reply to this conversation on ReviewNB OriolAbril commented on 2021-06-12T13:59:28Z I would add a cell with plot posterior using ref_vals to keep the P(weight < 0) = 0.1845 P(height < 0) = 0.0 info in a graphical way |
|
View / edit / reply to this conversation on ReviewNB OriolAbril commented on 2021-06-12T13:59:29Z what is male alpha? do we want to show i in the plot_pair? why wasn' in the old model? |
|
View / edit / reply to this conversation on ReviewNB OriolAbril commented on 2021-06-12T13:59:29Z is statsmodels still used?
almostmeenal commented on 2021-06-15T14:57:07Z nope, i'll remove it. also since we're using the graphviz figure i'll import that so its a known dependency |
|
Added several comments, this is great work |
|
View / edit / reply to this conversation on ReviewNB twiecki commented on 2021-06-13T08:22:46Z Line #2. plt.plot(x, y, "x") This should plot the outlier data. |
|
View / edit / reply to this conversation on ReviewNB twiecki commented on 2021-06-13T08:22:47Z Pretty obvious funnel, here you could link to https://twiecki.io/blog/2017/02/08/bayesian-hierchical-non-centered/ |
|
Agree on this. Something inspired on our overview here https://github.com/bambinos/bambi and one sentence saying it uses a formula for the model is enough I think. View entire conversation on ReviewNB |
|
I would also mention priors are automatically selected when the user does not specify them. Something in the same spirit than the block that used to start with "Since there are a couple of general linear models that are being" View entire conversation on ReviewNB |
|
View / edit / reply to this conversation on ReviewNB tomicapretto commented on 2021-06-14T00:27:14Z This is about the old behavior with PyMC3. I could help with some similar words but in a Bambi context, just let me know :) |
|
nope, i'll remove it. also since we're using the graphviz figure i'll import that so its a known dependency View entire conversation on ReviewNB |
thankyou @OriolAbril !!! |
|
I think this is great shape now. Can we mark it ready for merge? |
sure thing! you can do it, or else I'm on my phone rn i'll do it in afternoon, also thanks for the graphviz comment that looks cool and useful :)) |
|
Thanks @almostmeenal! |
|
View / edit / reply to this conversation on ReviewNB OriolAbril commented on 2021-06-16T08:54:28Z This will be the first contact with bambi for probably hundreds of people a month. I think this needs a link to bambi documentation more than a link to its paper. Having both is probably fine too, but I think this paper linked here is outdated. cc @tomicapretto @canyon289
Extra side note that unlike the above should not be implemented until after 3.11.3 when we start using myst. We could have a link to the documentation plus a clickable aloctavodia commented on 2021-06-16T09:00:52Z Right the correct link to the paper is https://arxiv.org/abs/2012.10754 and the documentation is here https://bambinos.github.io/bambi/master/index.html |
|
View / edit / reply to this conversation on ReviewNB OriolAbril commented on 2021-06-16T08:54:29Z The explanation is great and the link to the family object too. I also have a myst related extra note here (again, so not implement yet) that we could use intersphinx for that link so that if bambi decides to rename master to main, we'll only have to change a link in
almostmeenal commented on 2021-06-18T10:28:12Z keeping this comment unresolved for later |
|
View / edit / reply to this conversation on ReviewNB OriolAbril commented on 2021-06-16T08:54:30Z I think people won't know what |
|
View / edit / reply to this conversation on ReviewNB OriolAbril commented on 2021-06-16T08:54:31Z A link to a line in the source code on github doesn't feel right for the general introductory level of the content. It could link to https://bambinos.github.io/bambi/master/notebooks/getting_started.html#Specifying-priors or https://bambinos.github.io/bambi/master/api_reference.html#bambi.models.Model.set_priors |
|
Right the correct link to the paper is https://arxiv.org/abs/2012.10754 and the documentation is here https://bambinos.github.io/bambi/master/index.html View entire conversation on ReviewNB |
|
View / edit / reply to this conversation on ReviewNB aloctavodia commented on 2021-06-16T09:38:45Z I think |
|
View / edit / reply to this conversation on ReviewNB aloctavodia commented on 2021-06-16T09:38:46Z it should be Bambi |
|
View / edit / reply to this conversation on ReviewNB aloctavodia commented on 2021-06-16T09:38:47Z We can avoid doing
|
|
View / edit / reply to this conversation on ReviewNB aloctavodia commented on 2021-06-16T09:38:47Z
|
|
View / edit / reply to this conversation on ReviewNB aloctavodia commented on 2021-06-16T09:38:48Z Bambi/PyMC3 |
|
View / edit / reply to this conversation on ReviewNB aloctavodia commented on 2021-06-16T09:38:49Z unify: normal/Normal student T/student-t (also later in the example we have student t) |
|
View / edit / reply to this conversation on ReviewNB aloctavodia commented on 2021-06-16T09:38:49Z Maybe we should used a different prior than a Uniform distribution, like a HalfNormal |
|
View / edit / reply to this conversation on ReviewNB aloctavodia commented on 2021-06-16T09:38:50Z You can remove the plt.show() from this and the other cells |
|
keeping this comment unresolved for later View entire conversation on ReviewNB |
addresses issue #91
Note: I've just started hopefully, i'll finish it in a day or two
cc: @OriolAbril