Update first half of generalized linear models to "Best Practices" #148

KavyaJaiswal · 2021-04-29T08:11:05Z

Description

File: https://github.com/pymc-devs/pymc-examples/blob/main/examples/generalized_linear_models/GLM.ipynb

Addresses issue #91 and aims to advance the notebook to the 'Best Practices' State.

The new code uses the new numpy generator instead of the numpy random. The code also has an ArviZ related addition of return_inferencedata=True

The PR has a try... except clause in the code. It will ensure that users who have cloned pymc-examples repo will read their local copy of the data while also downloading the data from github for those who don't have a local copy.

try:
    sat_data = pd.read_csv("../data/Guber1999data.txt")
except:
    sat_data = pd.read_csv(pm.get_data("Guber1999data.txt"))

This PR also addresses the error message in the Hierarchical GLM section of the code. The error in the code is shown in the screenshots attached below. The fix for this error can be discussed and worked upon.

review-notebook-app · 2021-04-29T08:11:08Z

Check out this pull request on

See visual diffs & provide feedback on Jupyter Notebooks.

Powered by ReviewNB

review-notebook-app · 2021-04-29T21:27:30Z

View / edit / reply to this conversation on ReviewNB

OriolAbril commented on 2021-04-29T21:27:29Z
----------------------------------------------------------------

This is the old global seeding version of the numpy random number generator. You have to use the new random generator as shown in the references.

OriolAbril commented on 2021-05-01T20:43:59Z
----------------------------------------------------------------

Now a generator is initialized, but it is not stored nor used afterwards to generate the random samples. It should be stored (i.e. as rng) and then random samples should be generated with rng.normal, rng.poisson... there should be no instances of np.random.<distribution> anywhere in the code anymore.

KavyaJaiswal commented on 2021-05-03T10:54:44Z
----------------------------------------------------------------

I have made the required changes.

review-notebook-app · 2021-04-29T21:27:30Z

View / edit / reply to this conversation on ReviewNB

OriolAbril commented on 2021-04-29T21:27:30Z
----------------------------------------------------------------

Here return_inferencedata=True should be used in pm.sample. plot_posterior_predictive_glm works with both multitrace and inferencedata so everything else should work. Same goes for all other calls to pm.sample

KavyaJaiswal commented on 2021-05-03T10:55:31Z
----------------------------------------------------------------

Done!

review-notebook-app · 2021-04-29T21:27:31Z

View / edit / reply to this conversation on ReviewNB

OriolAbril commented on 2021-04-29T21:27:31Z
----------------------------------------------------------------

Here you have to use a try...except clause as indicated in https://github.com/pymc-devs/pymc3/wiki/PyMC3-Jupyter-Notebook-Style-Guide. This is completely independent from the glm issue in the cell below.

This cell does work and you have been able to execute it without problem. But right now it's downloading the data from github every time the cell is executed. If the users have cloned pymc-examples repo, we want to avoid that and use the local copy. This should be faster for those users as no download is needed in the cloned repo case, and will allow people with the cloned repo to run notebooks offline without problem which is currently not possible.

review-notebook-app · 2021-04-29T21:27:32Z

View / edit / reply to this conversation on ReviewNB

OriolAbril commented on 2021-04-29T21:27:32Z
----------------------------------------------------------------

Asked around for help with this error

OriolAbril commented on 2021-05-01T22:34:36Z
----------------------------------------------------------------

It is probably not worth it to fix those errors. Long term, bambi will replace the glm module, but the timeline is not clear yet. I would suggest working on the other comments as practice (together with the other PRs) and for now leaving this as is until we have had some time to plan the deprecation of the glm module

KavyaJaiswal commented on 2021-05-03T10:59:33Z
----------------------------------------------------------------

Fair enough.

review-notebook-app · 2021-04-29T21:27:33Z

View / edit / reply to this conversation on ReviewNB

OriolAbril commented on 2021-04-29T21:27:32Z
----------------------------------------------------------------

You need to install the watermark library for this to work, make sure you install it to the right environment, not to "general" or "base" ones.

KavyaJaiswal commented on 2021-05-03T10:58:36Z
----------------------------------------------------------------

Updated!

OriolAbril · 2021-05-01T20:44:00Z

Now a generator is initialized, but it is not stored nor used afterwards to generate the random samples. It should be stored (i.e. as rng) and then random samples should be generated with rng.normal, rng.poisson... there should be no instances of np.random.<distribution> anywhere in the code anymore.

View entire conversation on ReviewNB

OriolAbril · 2021-05-01T22:34:37Z

It is probably not worth it to fix those errors. Long term, bambi will replace the glm module, but the timeline is not clear yet. I would suggest working on the other comments as practice (together with the other PRs) and for now leaving this as is until we have had some time to plan the deprecation of the glm module

View entire conversation on ReviewNB

review-notebook-app · 2021-05-01T22:34:47Z

View / edit / reply to this conversation on ReviewNB

OriolAbril commented on 2021-05-01T22:34:46Z
----------------------------------------------------------------

The code should be try except directly, not be commented. The ellipsis should not be there either. They are in the guide because many notebooks need to pass extra arguments to read_csv. Also note that we have just updated the guide to show the specific exception that needs to be catched.

KavyaJaiswal commented on 2021-05-03T10:56:16Z
----------------------------------------------------------------

Updated accordingly.

OriolAbril commented on 2021-05-04T13:54:00Z
----------------------------------------------------------------

I would also remove the comment and use a specific exception as Marco pointed out and added to the style guide: https://github.com/pymc-devs/pymc3/wiki/PyMC3-Jupyter-Notebook-Style-Guide

KavyaJaiswal · 2021-05-02T10:59:30Z

I have added all the required changes.

KavyaJaiswal · 2021-05-03T10:54:45Z

I have made the required changes.

View entire conversation on ReviewNB

KavyaJaiswal · 2021-05-03T10:55:32Z

Done!

View entire conversation on ReviewNB

KavyaJaiswal · 2021-05-03T10:56:17Z

Updated accordingly.

View entire conversation on ReviewNB

KavyaJaiswal · 2021-05-03T10:58:37Z

Updated!

View entire conversation on ReviewNB

KavyaJaiswal · 2021-05-03T10:59:34Z

Fair enough.

View entire conversation on ReviewNB

OriolAbril · 2021-05-04T13:54:01Z

I would also remove the comment and use a specific exception as Marco pointed out and added to the style guide: https://github.com/pymc-devs/pymc3/wiki/PyMC3-Jupyter-Notebook-Style-Guide

View entire conversation on ReviewNB

OriolAbril

Two general comments.

First one is to update the PR title so when we merge it and it becomes a commit message, it will be informative of the changes, to do so it needs to mention the notebook that was modified. Most notebooks will be updated to use the new generator, so without that we'd end up with ~70 identical commit messages.

Second one is to remember to run pre-commit to ensure that CI passes. There is some guidance on that at https://github.com/pymc-devs/pymc3/wiki/PyMC3-Python-Code-Style

KavyaJaiswal · 2021-05-04T22:02:36Z

Updated the Pull Request with the suggested changes,

Replaced numpy.random. with the new numpy generator

8623a11

KavyaJaiswal changed the title ~~Replaced numpy.random with the new numpy generator #91~~ Replaced numpy.random with the new numpy generator Apr 29, 2021

KavyaJaiswal added 3 commits April 30, 2021 17:19

Replaced numpy.random with the new numpy random number generator

f51de03

Added comment for try catch statement

b58bb7b

Added final changes

eaed456

KavyaJaiswal changed the title ~~Replaced numpy.random with the new numpy generator~~ Replaced numpy.random with the new random generator May 1, 2021

Uncommented try catch clause

7ae9bc1

Final changes

8287473

OriolAbril reviewed May 4, 2021

View reviewed changes

KavyaJaiswal changed the title ~~Replaced numpy.random with the new random generator~~ Update generalized linear models to "Best Practices" State May 4, 2021

KavyaJaiswal added 2 commits May 5, 2021 02:53

Removed the comment

e7bcc59

CI test

5029b83

OriolAbril changed the title ~~Update generalized linear models to "Best Practices" State~~ Update first half of generalized linear models to "Best Practices" May 25, 2021

OriolAbril merged commit 5255505 into pymc-devs:main May 25, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update first half of generalized linear models to "Best Practices" #148

Update first half of generalized linear models to "Best Practices" #148

KavyaJaiswal commented Apr 29, 2021 •

edited

Loading

review-notebook-app bot commented Apr 29, 2021

review-notebook-app bot commented Apr 29, 2021 •

edited

Loading

review-notebook-app bot commented Apr 29, 2021 •

edited

Loading

review-notebook-app bot commented Apr 29, 2021

review-notebook-app bot commented Apr 29, 2021 •

edited

Loading

review-notebook-app bot commented Apr 29, 2021 •

edited

Loading

OriolAbril commented May 1, 2021

OriolAbril commented May 1, 2021

review-notebook-app bot commented May 1, 2021 •

edited

Loading

KavyaJaiswal commented May 2, 2021

KavyaJaiswal commented May 3, 2021

KavyaJaiswal commented May 3, 2021

KavyaJaiswal commented May 3, 2021

KavyaJaiswal commented May 3, 2021

KavyaJaiswal commented May 3, 2021

OriolAbril commented May 4, 2021

OriolAbril left a comment

KavyaJaiswal commented May 4, 2021

Update first half of generalized linear models to "Best Practices" #148

Update first half of generalized linear models to "Best Practices" #148

Conversation

KavyaJaiswal commented Apr 29, 2021 • edited Loading

Description

review-notebook-app bot commented Apr 29, 2021

review-notebook-app bot commented Apr 29, 2021 • edited Loading

review-notebook-app bot commented Apr 29, 2021 • edited Loading

review-notebook-app bot commented Apr 29, 2021

review-notebook-app bot commented Apr 29, 2021 • edited Loading

review-notebook-app bot commented Apr 29, 2021 • edited Loading

OriolAbril commented May 1, 2021

OriolAbril commented May 1, 2021

review-notebook-app bot commented May 1, 2021 • edited Loading

KavyaJaiswal commented May 2, 2021

KavyaJaiswal commented May 3, 2021

KavyaJaiswal commented May 3, 2021

KavyaJaiswal commented May 3, 2021

KavyaJaiswal commented May 3, 2021

KavyaJaiswal commented May 3, 2021

OriolAbril commented May 4, 2021

OriolAbril left a comment

Choose a reason for hiding this comment

KavyaJaiswal commented May 4, 2021

KavyaJaiswal commented Apr 29, 2021 •

edited

Loading

review-notebook-app bot commented Apr 29, 2021 •

edited

Loading

review-notebook-app bot commented Apr 29, 2021 •

edited

Loading

review-notebook-app bot commented Apr 29, 2021 •

edited

Loading

review-notebook-app bot commented Apr 29, 2021 •

edited

Loading

review-notebook-app bot commented May 1, 2021 •

edited

Loading