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

OriolAbril commented on 2021-05-04T11:58:34Z
----------------------------------------------------------------

Here there are also np.random... that should use rng instead.

KavyaJaiswal commented on 2021-05-05T12:15:13Z
----------------------------------------------------------------

Updated accordingly

View / edit / reply to this conversation on ReviewNB

OriolAbril commented on 2021-05-04T11:58:34Z
----------------------------------------------------------------

More np.random... that should be rng.

KavyaJaiswal commented on 2021-05-05T12:15:28Z
----------------------------------------------------------------

Updated

I'm currently working on the ArviZ related changes and will update it as soon as I complete them.

Updated accordingly

View entire conversation on ReviewNB

Updated

View entire conversation on ReviewNB

View / edit / reply to this conversation on ReviewNB

OriolAbril commented on 2021-05-06T22:05:16Z
----------------------------------------------------------------

The plot should try to generate a plot similar to the original one. Try for example something like:

az.plot_posterior(length_ppc, ref_val=ref_length, point_estimate=None, hdi_prob="hide")

You shoud be able to remove the ax.text and percentile too, plot_posterior will label everything. All arguments and their description are at: https://arviz-devs.github.io/arviz/api/generated/arviz.plot_posterior.html

OriolAbril commented on 2021-10-13T02:23:56Z
----------------------------------------------------------------

Here use the provided code please and update the ax.text and so on accordingly.

View / edit / reply to this conversation on ReviewNB

KavyaJaiswal commented on 2021-05-07T10:32:33Z
----------------------------------------------------------------

What exactly should be my approach to fix this warning?

OriolAbril commented on 2021-05-07T15:21:42Z
----------------------------------------------------------------

pp_lenght is currently a 2d array with dims draw, month, which works fine by now, but this does not follow ArviZ convention on shape which is chain, draw for 2d data. In a future ArviZ verision, hdi will modify its behaviour, and instead of returning a 1d array of length n_months it will return a scalar.

The ideal situation is to pass array objects with dims chain, draw, *shape, . This should happen automatically if storing the posterior predictive as a group in inferencedata like it's done in the rugby example. Using xarray things will require some minor changes to plotting which should then be similar to the hdi plots in the radon example (multilevel modeling primer one)

KavyaJaiswal commented on 2021-10-12T15:50:04Z
----------------------------------------------------------------

Please guide me to resolve this part.

OriolAbril commented on 2021-10-13T02:23:02Z
----------------------------------------------------------------

I think it will be fixed automatically if using InferenceData as indicated above

pp_lenght is currently a 2d array with dims draw, month, which works fine by now, but this does not follow ArviZ convention on shape which is chain, draw for 2d data. In a future ArviZ verision, hdi will modify its behaviour, and instead of returning a 1d array of length n_months it will return a scalar.

The ideal situation is to pass array objects with dims chain, draw, *shape, . This should happen automatically if storing the posterior predictive as a group in inferencedata like it's done in the rugby example. Using xarray things will require some minor changes to plotting which should then be similar to the hdi plots in the radon example (multilevel modeling primer one)

View entire conversation on ReviewNB

@KavyaJaiswal any chance of pushing this to completion?

I'm really sorry! I am currently busy with my Outreachy internship and don't think I will be able to work on this till August.

@KavyaJaiswal let us know if you need any help to finish this or if you prefer someone else take over 😄

@OriolAbril If it's urgent, someone can definitely take over. If not, I'm finishing my Outreachy internship on 31st August and I can then finish this.

It's not urgent, just wanted to have an idea so we can get it done roughly by the end of September

Sure

Can I still work on this PR? @OriolAbril

Yes! You will need to update the PR with the latest changes in main and take a look at the improved style guide https://github.com/pymc-devs/pymc/wiki/PyMC3-Jupyter-Notebook-Style-Guide. Your PR has been frozen but the repo has had a lot of work done to it in many areas! Let us know if you need any help

@OriolAbril I believe you sent the wrong link by mistake.

fixed, sorry about the confusion

Please guide me to resolve this part.

View entire conversation on ReviewNB

I think it will be fixed automatically if using InferenceData as indicated above

View entire conversation on ReviewNB

Here use the provided code please and update the ax.text and so on accordingly.

View entire conversation on ReviewNB

View / edit / reply to this conversation on ReviewNB

OriolAbril commented on 2021-10-13T02:24:01Z
----------------------------------------------------------------

Line #16.            traces.append(pm.sample(return_inferencedata=False))

this should use return_inferencedata=True

View / edit / reply to this conversation on ReviewNB

OriolAbril commented on 2021-10-13T02:24:02Z
----------------------------------------------------------------

Line #12.        trace = pm.sample(return_inferencedata=False, tune=2000)

same here, we should always return inferencedata.

View / edit / reply to this conversation on ReviewNB

OriolAbril commented on 2021-10-13T02:24:03Z
----------------------------------------------------------------

Line #14.        trace_babies = pm.sample(tune=2000, return_inferencedata=False)

same here, return InferenceData. I think the two above won't require any extra changes, this one will need some updates below, I am comenting there too

View / edit / reply to this conversation on ReviewNB

OriolAbril commented on 2021-10-13T02:24:05Z
----------------------------------------------------------------

Line #2.        pp_length = pm.sample_posterior_predictive(trace_babies)["length"]

here the result should be added to the inferencedata from above, you can follow the pattern in https://pymc-examples.readthedocs.io/en/latest/case_studies/rugby_analytics.html#results. There will therefore not be a pp_length anymore, but you can re-define it for convenience as done in the rugby one too more or less.

@KavyaJaiswal I took the liberty of taking over and finish it so we can merge it in the following days as we have just released the beta version for the pymc 4.0

Thank you so much @OriolAbril for finishing this merge request!

View / edit / reply to this conversation on ReviewNB

michaelosthege commented on 2021-12-19T18:11:18Z
----------------------------------------------------------------

We can probably do without tune, draw and target_accept, since this model is simple enough to work every time.

View / edit / reply to this conversation on ReviewNB

michaelosthege commented on 2021-12-19T18:11:19Z
----------------------------------------------------------------

Let's move this up into the cell where the model was defined