Update weibull_aft notebook #4020

AmitKus · 2020-07-19T04:18:44Z

Fixed: AttributeError: module 'statsmodels' has no attribute 'datasets'
Fixed: FutureWarnings
Updated the notebook to follow the consistent style (https://github.com/pymc-devs/pymc3/wiki/PyMC's-Jupyter-Notebook-Style)

review-notebook-app · 2020-07-19T04:18:50Z

Check out this pull request on

Review Jupyter notebook visual diffs & provide feedback on notebooks.

Powered by ReviewNB

AlexAndorra

Thanks @AmitKus, this looks good! I just posted some comments below -- should be easy to address.
Tests are failing because of a Matplotlib issue, unrelated to your changes. Once #4023 is merged, you can rebase your current branch off master to take the changes into account and the tests should pass 😉

review-notebook-app · 2020-07-20T20:37:05Z

View / edit / reply to this conversation on ReviewNB

AlexAndorra commented on 2020-07-20T20:37:05Z
----------------------------------------------------------------

az.plot_trace(data_1, var_names=["alpha", "beta"]) outside of the model context would be better than pm.traceplot(trace_1, var_names=["alpha", "beta"]) inside the model context

review-notebook-app · 2020-07-20T20:37:06Z

View / edit / reply to this conversation on ReviewNB

AlexAndorra commented on 2020-07-20T20:37:06Z
----------------------------------------------------------------

Use az.summary(data_1, var_names=["alpha", "beta"], round_to=2) instead

review-notebook-app · 2020-07-20T20:37:07Z

View / edit / reply to this conversation on ReviewNB

AlexAndorra commented on 2020-07-20T20:37:07Z
----------------------------------------------------------------

Same comment as model 1 for plot_trace

review-notebook-app · 2020-07-20T20:37:08Z

View / edit / reply to this conversation on ReviewNB

AlexAndorra commented on 2020-07-20T20:37:07Z
----------------------------------------------------------------

Same comment as model 1 for summary

review-notebook-app · 2020-07-20T20:37:08Z

View / edit / reply to this conversation on ReviewNB

AlexAndorra commented on 2020-07-20T20:37:08Z
----------------------------------------------------------------

Same comment as model 1 for plot_trace

review-notebook-app · 2020-07-20T20:37:09Z

View / edit / reply to this conversation on ReviewNB

AlexAndorra commented on 2020-07-20T20:37:09Z
----------------------------------------------------------------

Same comment as model 1 for summary

AmitKus · 2020-07-21T05:07:15Z

Thanks @AmitKus, this looks good! I just posted some comments below -- should be easy to address.
Tests are failing because of a Matplotlib issue, unrelated to your changes. Once #4023 is merged, you can rebase your current branch off master to take the changes into account and the tests should pass wink

Thanks @AlexAndorra for taking out time to review the changes. I made the changes you had suggested.

codecov · 2020-07-21T06:15:07Z

Codecov Report

Merging #4020 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #4020   +/-   ##
=======================================
  Coverage   86.77%   86.77%           
=======================================
  Files          88       88           
  Lines       14134    14134           
=======================================
  Hits        12265    12265           
  Misses       1869     1869

AlexAndorra · 2020-07-21T11:03:09Z

Thanks @AmitKus ! However, I don't think you rebased your branch -- you just picked up Robert's changes and integrated them into your PR. But these changes are already on the latest master. See what I mean?
I'm no git wizard, but I think you need to rebase your PR, i.e base it on the latest version of master, not on the one you originally based it on. That way, the MPL issue will already be taken care of. Note that you may have to revert your changes and add them back again but I'm not sure.
Sorry if this is confusing but you'll get the hang of it with time 😉

…sets' 2. Fixed: FutureWarnings 3. Updated the notebook to follow the consistent style using arviz functions directly to plot trace and summary 1. Fixed: AttributeError: module 'statsmodels' has no attribute 'datasets' 2. Fixed: FutureWarnings 3. Updated the notebook to follow the consistent style using arviz functions directly

AmitKus · 2020-07-21T21:32:06Z

Thanks @AmitKus ! However, I don't think you rebased your branch -- you just picked up Robert's changes and integrated them into your PR. But these changes are already on the latest master. See what I mean?
I'm no git wizard, but I think you need to rebase your PR, i.e base it on the latest version of master, not on the one you originally based it on. That way, the MPL issue will already be taken care of. Note that you may have to revert your changes and add them back again but I'm not sure.
Sorry if this is confusing but you'll get the hang of it with time wink

Thanks for your patience @AlexAndorra :)
I followed the link and rebased by PR. Hopefully this works!

AlexAndorra

Well done @AmitKus, the git log looks all good now 👏
This is almost ready -- just left a few comments to address before merging 😉

review-notebook-app · 2020-07-22T08:55:21Z

View / edit / reply to this conversation on ReviewNB

AlexAndorra commented on 2020-07-22T08:55:21Z
----------------------------------------------------------------

It seems that pandas and patsy are imported but not used, in which case it's better to delete their import lines (ignore this if I'm mistaken)

review-notebook-app · 2020-07-22T08:55:22Z

View / edit / reply to this conversation on ReviewNB

AlexAndorra commented on 2020-07-22T08:55:21Z
----------------------------------------------------------------

Sorry, just thought about something I should have told in my previous review: actually you can tell sample to directly give back an InferenceData object, instead of having to then call az.from_pymc3 . Just use: data_1 = pm.sample(target_accept=0.9, init="adapt_diag", return_inferencedata=True) . Note that I deleted tune and draws because their default is already 1000.
Accordingly, change the comment to: # Change init to avoid divergences

These comments are valid for all the other models ;)

AmitKus commented on 2020-07-22T17:39:26Z
----------------------------------------------------------------

Hi Alex, Looks like the default for tune and draws is 500, so made all the changes that you recommended except deleting draws and tune.

Thanks,

Amit

review-notebook-app · 2020-07-22T08:55:23Z

View / edit / reply to this conversation on ReviewNB

AlexAndorra commented on 2020-07-22T08:55:22Z
----------------------------------------------------------------

Use round_to=2 arg instead of .round(2) -- more idiomatic ;)

This comment is valid for all calls to az.summary

…ymc3 2. Other minor formatting

AmitKus · 2020-07-22T17:39:27Z

Hi Alex, Looks like the default for tune and draws is 500, so made all the changes that you recommended except deleting draws and tune.

Thanks,

Amit

View entire conversation on ReviewNB

AlexAndorra · 2020-07-22T18:27:35Z

Thanks @AmitKus! This is weird though, the new default is 1000 for both tune and draws.
Which version of PyMC are you running? Can you post a screenshot of the progress bar when using the defaults please?

AmitKus · 2020-07-23T03:33:01Z

Hey Alex, I just looked at the documentation for version 3.8 here: https://docs.pymc.io/api/inference.html

If this has changed in the recent version then I'll update the code to remove tune and draw.

Sampling¶

pymc3.sampling.sample(draws=500, step=None, init='auto', n_init=200000, start=None, trace=None, chain_idx=0, chains=None, cores=None, tune=500, progressbar=True, model=None, random_seed=None, discard_tuned_samples=True, compute_convergence_checks=True, **kwargs)¶

AlexAndorra · 2020-07-23T08:55:54Z

Aaaaah ok, I see the confusion! Docs are actually not up-to-date with the latest version of the code yet (we're still updating some notebooks, as this one, so that they appear on the website when we compile the docs).
So, when running this NB with 3.9.2, you should be able to just pm.sample and the defaults of tune and draws should be 1000 👌

… they were using specifying the default values.

AmitKus · 2020-07-24T03:10:30Z

Aaaaah ok, I see the confusion! Docs are actually not up-to-date with the latest version of the code yet (we're still updating some notebooks, as this one, so that they appear on the website when we compile the docs).
So, when running this NB with 3.9.2, you should be able to just pm.sample and the defaults of tune and draws should be 1000 ok_hand

I see. Made the changes to remove the tune and draws.

AlexAndorra

This is super nice now 🎉 Thanks for your help @AmitKus !

AmitKus · 2020-07-24T18:41:57Z

This is super nice now tada Thanks for your help @AmitKus !

Great! Thanks for all the help @AlexAndorra

AlexAndorra requested changes Jul 20, 2020

View reviewed changes

AmitKus force-pushed the weibull-aft branch from 59aaa72 to a8b5dfc Compare July 21, 2020 20:35

AlexAndorra requested changes Jul 22, 2020

View reviewed changes

1. directly get inference object from sample rather than using from_p…

8d9e987

…ymc3 2. Other minor formatting

remove the 'tune' and 'draws' parameters from sample function call as…

08127d7

… they were using specifying the default values.

AlexAndorra approved these changes Jul 24, 2020

View reviewed changes

AlexAndorra merged commit 6f8a0f7 into pymc-devs:master Jul 24, 2020

AmitKus deleted the weibull-aft branch July 24, 2020 18:41

kyleabeauchamp added this to the 3.9.3 milestone Jul 28, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update weibull_aft notebook #4020

Update weibull_aft notebook #4020

AmitKus commented Jul 19, 2020 •

edited by AlexAndorra

Loading

review-notebook-app bot commented Jul 19, 2020

AlexAndorra left a comment

review-notebook-app bot commented Jul 20, 2020

review-notebook-app bot commented Jul 20, 2020

review-notebook-app bot commented Jul 20, 2020

review-notebook-app bot commented Jul 20, 2020

review-notebook-app bot commented Jul 20, 2020

review-notebook-app bot commented Jul 20, 2020

AmitKus commented Jul 21, 2020

codecov bot commented Jul 21, 2020 •

edited

Loading

AlexAndorra commented Jul 21, 2020

AmitKus commented Jul 21, 2020

AlexAndorra left a comment

review-notebook-app bot commented Jul 22, 2020 •

edited by AlexAndorra

Loading

review-notebook-app bot commented Jul 22, 2020 •

edited

Loading

review-notebook-app bot commented Jul 22, 2020

AmitKus commented Jul 22, 2020

AlexAndorra commented Jul 22, 2020

AmitKus commented Jul 23, 2020 •

edited

Loading

AlexAndorra commented Jul 23, 2020

AmitKus commented Jul 24, 2020

AlexAndorra left a comment

AmitKus commented Jul 24, 2020

Update weibull_aft notebook #4020

Update weibull_aft notebook #4020

Conversation

AmitKus commented Jul 19, 2020 • edited by AlexAndorra Loading

review-notebook-app bot commented Jul 19, 2020

AlexAndorra left a comment

Choose a reason for hiding this comment

review-notebook-app bot commented Jul 20, 2020

review-notebook-app bot commented Jul 20, 2020

review-notebook-app bot commented Jul 20, 2020

review-notebook-app bot commented Jul 20, 2020

review-notebook-app bot commented Jul 20, 2020

review-notebook-app bot commented Jul 20, 2020

AmitKus commented Jul 21, 2020

codecov bot commented Jul 21, 2020 • edited Loading

Codecov Report

AlexAndorra commented Jul 21, 2020

AmitKus commented Jul 21, 2020

AlexAndorra left a comment

Choose a reason for hiding this comment

review-notebook-app bot commented Jul 22, 2020 • edited by AlexAndorra Loading

review-notebook-app bot commented Jul 22, 2020 • edited Loading

review-notebook-app bot commented Jul 22, 2020

AmitKus commented Jul 22, 2020

AlexAndorra commented Jul 22, 2020

AmitKus commented Jul 23, 2020 • edited Loading

Sampling¶

AlexAndorra commented Jul 23, 2020

AmitKus commented Jul 24, 2020

AlexAndorra left a comment

Choose a reason for hiding this comment

AmitKus commented Jul 24, 2020

AmitKus commented Jul 19, 2020 •

edited by AlexAndorra

Loading

codecov bot commented Jul 21, 2020 •

edited

Loading

review-notebook-app bot commented Jul 22, 2020 •

edited by AlexAndorra

Loading

review-notebook-app bot commented Jul 22, 2020 •

edited

Loading

AmitKus commented Jul 23, 2020 •

edited

Loading