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

cluhmann commented on 2022-06-05T15:59:27Z
----------------------------------------------------------------

Per the Jupyter style guide, the preamble should be like this (omitting the figure format line):

RANDOM_SEED = 8927
rng = np.random.default_rng(RANDOM_SEED)
az.style.use("arviz-darkgrid")

drbenvincent commented on 2022-06-05T16:54:13Z
----------------------------------------------------------------

Done, in upcoming commit

View / edit / reply to this conversation on ReviewNB

cluhmann commented on 2022-06-05T15:59:28Z
----------------------------------------------------------------

Do we want to keep this? It's quite old and the use of pm.MutableData below renders this somewhat obsolete.

drbenvincent commented on 2022-06-05T16:54:25Z
----------------------------------------------------------------

removing this in upcoming commit

View / edit / reply to this conversation on ReviewNB

cluhmann commented on 2022-06-05T15:59:30Z
----------------------------------------------------------------

I think it's convention to index into InferenceData objects like this? I could be wrong and it probably doesn't matter too much.

idata.posterior_predictive['obs']mean(dim=["chain", "draw"])

drbenvincent commented on 2022-06-05T16:54:32Z
----------------------------------------------------------------

Agreed

View / edit / reply to this conversation on ReviewNB

cluhmann commented on 2022-06-05T15:59:30Z
----------------------------------------------------------------

Just a naming issue, but should the variables x1 and x2 be b1 and b1 or am I confused?

drbenvincent commented on 2022-06-05T16:56:55Z
----------------------------------------------------------------

Doesn't need to be because I used labels for the coefficient names. But here I agree, so I've changed them to b0, b1, b2, b1:b2

cluhmann commented on 2022-06-05T19:29:57Z
----------------------------------------------------------------

I guess I meant that x1 here (i.e., idata.posterior.b.sel(coeffs="b1").mean().data) is the mean posterior of a coefficient that is labeled "b1", which is then multiplied by the "x1" taken from the grid (i.e., x1_grid). So I guess I assumed that idata.posterior.b.sel(coeffs="b1").mean().data would be stored in a variable named b1 rather than x1.

Also, as in the comment resolved above, I tend to do: idata.posterior["b"] rather than idata.posterior.b.

View / edit / reply to this conversation on ReviewNB

cluhmann commented on 2022-06-05T15:59:31Z
----------------------------------------------------------------

Not sure what "it" is in "storing and computing it inside the model definition". Complete distribution of these metrics?

drbenvincent commented on 2022-06-05T16:58:31Z
----------------------------------------------------------------

I've removed the last sentence

Done, in upcoming commit

View entire conversation on ReviewNB

removing this in upcoming commit

View entire conversation on ReviewNB

Agreed

View entire conversation on ReviewNB

Doesn't need to be because I used labels for the coefficient names. But here I agree, so I've changed them to b0, b1, b2, b1:b2

View entire conversation on ReviewNB

I've removed the last sentence

View entire conversation on ReviewNB

I guess I meant that x1 here (i.e., idata.posterior.b.sel(coeffs="b1").mean().data) is the mean posterior of a coefficient that is labeled "b1", which is then multiplied by the "x1" taken from the grid (i.e., x1_grid). So I guess I assumed that idata.posterior.b.sel(coeffs="b1").mean().data would be stored in a variable named b1 rather than x1.

Also, as in the comment resolved above, I tend to do: idata.posterior["b"] rather than idata.posterior.b.

View entire conversation on ReviewNB

View / edit / reply to this conversation on ReviewNB

juanitorduz commented on 2022-06-05T20:33:25Z
----------------------------------------------------------------

Line #2.    sns_c_div = sns.diverging_palette(240, 10, n=2)

When I wrote this notebook I was using the ugly sns_c[0] trick to get seaborn's colours instead of using the more convenient "C0" , which I think is much less confusing. So I would suggest to change it. Sorry, this one is on me :/

drbenvincent commented on 2022-06-05T21:36:23Z
----------------------------------------------------------------

Have moved to just using defaults throughout. It doesn't look as good, but do feel free to hold me accountable to improve it and stop me from being lazy

Have moved to just using defaults throughout. It doesn't look as good, but do feel free to hold me accountable to improve it and stop me from being lazy

View entire conversation on ReviewNB

Any final comments @juanitorduz? Or happy for me to merge this?

LGTM 🚀

@OriolAbril I seem to have an approval from @cluhmann, but it says merging is blocked and that I don't have an approval.