Update VI Bayesian NN notebook to best practices #221

chiral-carbon · 2021-09-01T14:58:32Z

Addresses issue #131 and aims to advance it to best practices

review-notebook-app · 2021-09-01T14:58:36Z

Check out this pull request on

See visual diffs & provide feedback on Jupyter Notebooks.

Powered by ReviewNB

review-notebook-app · 2021-09-01T15:02:47Z

View / edit / reply to this conversation on ReviewNB

chiral-carbon commented on 2021-09-01T15:02:46Z
----------------------------------------------------------------

I commented this out for now, but would we require it when we are already setting the seed in the beginning?

OriolAbril commented on 2021-09-08T15:56:09Z
----------------------------------------------------------------

rng = np.random.default_rng(RANDOM_SEED)

only sets the seed of rng. Any other code that doesn't use rng is completely unaffected.

This is one of the main reasons that the np.random.seed was changed and is now legacy. It set a global seed that affected any calls to np.random unless another random state was created manually so it had effects and "produced" bugs because people did not realize their code was being affected by the seed.

chiral-carbon commented on 2021-09-09T08:38:12Z
----------------------------------------------------------------

oh okay. in thats case I do get an error here,

ImportError: cannot import name 'MRG_RandomStreams' from 'pymc3.theanof'

I think it is because of compatibility issues between pymc3 >= 3.10 and theano?

OriolAbril commented on 2021-09-09T11:01:47Z
----------------------------------------------------------------

It looks like it could be fixed with a call to tt_rng before set_tt_rng, but I think we can just remove it and pass a random seed to the sampling methods.

OriolAbril · 2021-09-08T15:56:10Z

rng = np.random.default_rng(RANDOM_SEED)

only sets the seed of rng. Any other code that doesn't use rng is completely unaffected.

This is one of the main reasons that the np.random.seed was changed and is now legacy. It set a global seed that affected any calls to np.random unless another random state was created manually so it had effects and "produced" bugs because people did not realize their code was being affected by the seed.

View entire conversation on ReviewNB

review-notebook-app · 2021-09-08T15:59:13Z

View / edit / reply to this conversation on ReviewNB

OriolAbril commented on 2021-09-08T15:59:13Z
----------------------------------------------------------------

Line #3.    ax.scatter(X[Y == 1, 0], X[Y == 1, 1], color="C3", label="Class 1")

I personally prefer c0-c1 as it has more contrast, but both options are fine

review-notebook-app · 2021-09-08T15:59:14Z

View / edit / reply to this conversation on ReviewNB

OriolAbril commented on 2021-09-08T15:59:14Z
----------------------------------------------------------------

Line #12.            "train_cols": np.arange(len(X[:][0])),

I would keep the X.shape[1] instead of this len

review-notebook-app · 2021-09-08T15:59:14Z

View / edit / reply to this conversation on ReviewNB

OriolAbril commented on 2021-09-08T15:59:14Z
----------------------------------------------------------------

Line #42.            out = pm.Bernoulli(

this should also have the dimensions annotated

chiral-carbon commented on 2021-09-09T08:58:58Z
----------------------------------------------------------------

what dimension would this have though?

OriolAbril commented on 2021-09-09T11:10:17Z
----------------------------------------------------------------

coords = {
    "hidden_layer_1": np.arange(n_hidden),
    "hidden_layer_2": np.arange(n_hidden),
    "train_col": np.arange(X.shape[1]),
    "obs_id": np.arange(X.shape[0])
}

I'd do something like that. Here the number of nodes in each layer are independent, yet coincidentally the same so i'd not use bis. As we don't know much/anything about the observations or their collection process, I just use obs_id as fallback.

review-notebook-app · 2021-09-08T15:59:15Z

View / edit / reply to this conversation on ReviewNB

OriolAbril commented on 2021-09-08T15:59:15Z
----------------------------------------------------------------

format with less digits, 3-4 at the very most I'd say. 1-2 already perfectly fine

chiral-carbon · 2021-09-09T08:38:13Z

oh okay. in thats case I do get an error here,

ImportError: cannot import name 'MRG_RandomStreams' from 'pymc3.theanof'

I think it is because of compatibility issues between pymc3 >= 3.10 and theano?

View entire conversation on ReviewNB

chiral-carbon · 2021-09-09T08:58:59Z

what dimension would this have though?

View entire conversation on ReviewNB

OriolAbril · 2021-09-09T11:01:47Z

It looks like it could be fixed with a call to tt_rng before set_tt_rng, but I think we can just remove it and pass a random seed to the sampling methods.

View entire conversation on ReviewNB

OriolAbril · 2021-09-09T11:10:18Z

coords = {
    "hidden_layer_1": np.arange(n_hidden),
    "hidden_layer_2": np.arange(n_hidden),
    "train_col": np.arange(X.shape[1]),
    "obs_id": np.arange(X.shape[0])
}

I'd do something like that. Here the number of nodes in each layer are independent, yet coincidentally the same so i'd not use bis. As we don't know much/anything about the observations or their collection process, I just use obs_id as fallback.

View entire conversation on ReviewNB

review-notebook-app · 2021-09-10T20:44:04Z

View / edit / reply to this conversation on ReviewNB

Sayam753 commented on 2021-09-10T20:44:03Z
----------------------------------------------------------------

I think the problem lies here -

coords = {
    "hidden_layer_1": np.arange(n_hidden),
    "hidden_layer_2": np.arange(n_hidden),
    "train_col": np.arange(X.shape[1]),
    "obs_id": np.arange(X.shape[0])
}

X has shape (1000, 2), but X_train has shape (500, 2). It is X_train that we use for training.

So, the right coords will be -

coords = {
    "hidden_layer_1": np.arange(n_hidden),
    "hidden_layer_2": np.arange(n_hidden),
    "train_col": np.arange(X_train.shape[1]),
    "obs_id": np.arange(X_train.shape[0])
}

I wonder what will happen if we pass X_test of shape not of shape (500, 2), but something else, like (400, 2) :thinking_face:

Sayam753 commented on 2021-09-10T20:56:31Z
----------------------------------------------------------------

Minor nitpick -

Looking at the warning, @OriolAbril do we promote plotting trace in PyMC3's model context?

OriolAbril commented on 2021-09-10T22:37:42Z
----------------------------------------------------------------

Right, because the original code uses X but should use X_train. There wasn't an error because the columns are the same. Now that we take cols and rows we do get an error.

No, plotting should not happen inside the model context. The warning is because we are giving ArviZ a non InferenceData input, so it first has to convert to idata. It's the conversion what should happen inside the model context, and it should be explicit. After that, plotting will work without warnings outside the model context.

Sayam753 · 2021-09-10T20:56:32Z

Minor nitpick -

Looking at the warning, @OriolAbril do we promote plotting trace in PyMC3's model context?

View entire conversation on ReviewNB

OriolAbril · 2021-09-10T22:37:43Z

Right, because the original code uses X but should use X_train. There wasn't an error because the columns are the same. Now that we take cols and rows we do get an error.

No, plotting should not happen inside the model context. The warning is because we are giving ArviZ a non InferenceData input, so it first has to convert to idata. It's the conversion what should happen inside the model context, and it should be explicit. After that, plotting will work without warnings outside the model context.

View entire conversation on ReviewNB

review-notebook-app · 2021-09-19T03:22:58Z

View / edit / reply to this conversation on ReviewNB

Sayam753 commented on 2021-09-19T03:22:58Z
----------------------------------------------------------------

How do I interpret the output of this cell?

OriolAbril commented on 2021-10-13T05:04:40Z
----------------------------------------------------------------

this is a forgotten debug print that made it to the arviz release, rerunning with 0.11.4 will get rid of this message

OriolAbril · 2021-10-13T05:04:41Z

this is a forgotten debug print that made it to the arviz release, rerunning with 0.11.4 will get rid of this message

View entire conversation on ReviewNB

Sayam753

LGTM

OriolAbril

I'd also suggest rerunning with latest arviz to get rid of those debug prints. But at the very least it needs to include the post directive. It should not be merged without it

chiral-carbon · 2021-10-21T17:07:37Z

@OriolAbril I have fixed all the broken links, I think this could be merged now

OriolAbril · 2021-10-22T23:41:31Z

There is still a broken ADVI link in the summary section.

chiral-carbon · 2021-10-23T10:06:51Z

In the next steps section, should I add a line where Theano is mentioned about it being continued as Aesara?

OriolAbril · 2021-10-24T04:39:48Z

we can directly update theano to aesara when running with v4

chiral-carbon added the outreachy2021 label Sep 3, 2021

Sayam753 approved these changes Oct 17, 2021

View reviewed changes

OriolAbril requested changes Oct 17, 2021

View reviewed changes

chiral-carbon added 5 commits October 20, 2021 18:17

best practices updates

ca81419

updates: coords naming

176b03a

fix coords

4c0ea45

style guide changes

1956443

rerun precommit

292bf13

chiral-carbon force-pushed the vi-bayesianNN branch from d48a348 to 292bf13 Compare October 20, 2021 12:51

chiral-carbon added 3 commits October 20, 2021 18:59

fixes

a2897a0

author names in ref

e0deda6

run with arviz v0.11.4

81a25b4

chiral-carbon requested a review from OriolAbril October 20, 2021 15:09

fix links

d4afe49

chiral-carbon force-pushed the vi-bayesianNN branch from bde4a7b to d4afe49 Compare October 21, 2021 09:43

fix broken ADVI link

f863fe1

OriolAbril approved these changes Oct 24, 2021

View reviewed changes

OriolAbril merged commit ff2af9f into pymc-devs:main Oct 24, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update VI Bayesian NN notebook to best practices #221

Update VI Bayesian NN notebook to best practices #221

chiral-carbon commented Sep 1, 2021

review-notebook-app bot commented Sep 1, 2021

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

edited

Loading

OriolAbril commented Sep 8, 2021

review-notebook-app bot commented Sep 8, 2021 •

edited

Loading

review-notebook-app bot commented Sep 8, 2021 •

edited

Loading

review-notebook-app bot commented Sep 8, 2021 •

edited

Loading

review-notebook-app bot commented Sep 8, 2021

chiral-carbon commented Sep 9, 2021

chiral-carbon commented Sep 9, 2021

OriolAbril commented Sep 9, 2021

OriolAbril commented Sep 9, 2021

review-notebook-app bot commented Sep 10, 2021 •

edited

Loading

Sayam753 commented Sep 10, 2021

OriolAbril commented Sep 10, 2021

review-notebook-app bot commented Sep 19, 2021 •

edited

Loading

OriolAbril commented Oct 13, 2021

Sayam753 left a comment

OriolAbril left a comment

chiral-carbon commented Oct 21, 2021

OriolAbril commented Oct 22, 2021

chiral-carbon commented Oct 23, 2021

OriolAbril commented Oct 24, 2021

Update VI Bayesian NN notebook to best practices #221

Update VI Bayesian NN notebook to best practices #221

Conversation

chiral-carbon commented Sep 1, 2021

review-notebook-app bot commented Sep 1, 2021

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

OriolAbril commented Sep 8, 2021

review-notebook-app bot commented Sep 8, 2021 • edited Loading

review-notebook-app bot commented Sep 8, 2021 • edited Loading

review-notebook-app bot commented Sep 8, 2021 • edited Loading

review-notebook-app bot commented Sep 8, 2021

chiral-carbon commented Sep 9, 2021

chiral-carbon commented Sep 9, 2021

OriolAbril commented Sep 9, 2021

OriolAbril commented Sep 9, 2021

review-notebook-app bot commented Sep 10, 2021 • edited Loading

Sayam753 commented Sep 10, 2021

OriolAbril commented Sep 10, 2021

review-notebook-app bot commented Sep 19, 2021 • edited Loading

OriolAbril commented Oct 13, 2021

Sayam753 left a comment

Choose a reason for hiding this comment

OriolAbril left a comment

Choose a reason for hiding this comment

chiral-carbon commented Oct 21, 2021

OriolAbril commented Oct 22, 2021

chiral-carbon commented Oct 23, 2021

OriolAbril commented Oct 24, 2021

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

edited

Loading

review-notebook-app bot commented Sep 8, 2021 •

edited

Loading

review-notebook-app bot commented Sep 8, 2021 •

edited

Loading

review-notebook-app bot commented Sep 8, 2021 •

edited

Loading

review-notebook-app bot commented Sep 10, 2021 •

edited

Loading

review-notebook-app bot commented Sep 19, 2021 •

edited

Loading