Circular kernel #4082

ferrine · 2020-09-06T15:04:20Z

In this PR Circular kernel in introduced to work on exotic domains, e.g. Unit Circle

what are the (breaking) changes that this PR makes?
important background, or details about the implementation
are the changes—especially new features—covered by tests and docstrings?
consider adding/updating relevant example notebooks
right before it's ready to merge, mention the PR in the RELEASE-NOTES.md

pymc3.gp.cov.Circular is a new class to work with. The example can be found in the supplementary notebook.

review-notebook-app · 2020-09-06T15:04:24Z

Check out this pull request on

Review Jupyter notebook visual diffs & provide feedback on notebooks.

Powered by ReviewNB

codecov · 2020-09-06T16:03:17Z

Codecov Report

Merging #4082 into master will increase coverage by 1.63%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #4082      +/-   ##
==========================================
+ Coverage   88.74%   90.38%   +1.63%     
==========================================
  Files          89       89              
  Lines       14037    17786    +3749     
==========================================
+ Hits        12457    16075    +3618     
- Misses       1580     1711     +131

Impacted Files	Coverage Δ
pymc3/gp/cov.py	`99.22% <100.00%> (+1.26%)`	⬆️
pymc3/math.py	`66.16% <0.00%> (-2.32%)`	⬇️
pymc3/plots/posteriorplot.py	`19.23% <0.00%> (-1.83%)`	⬇️
pymc3/step_methods/elliptical_slice.py	`94.33% <0.00%> (-0.79%)`	⬇️
pymc3/tests/sampler_fixtures.py	`96.12% <0.00%> (-0.65%)`	⬇️
pymc3/distributions/transforms.py	`97.04% <0.00%> (-0.41%)`	⬇️
pymc3/step_methods/gibbs.py	`39.65% <0.00%> (-0.35%)`	⬇️
pymc3/variational/callbacks.py	`95.89% <0.00%> (-0.03%)`	⬇️
pymc3/ode/utils.py	`100.00% <0.00%> (ø)`
pymc3/tests/conftest.py	`100.00% <0.00%> (ø)`
... and 42 more

AlexAndorra

Thanks a lot @ferrine , super exciting to get this into PyMC3 🎉
This looks good to me, I just left a few comments below, and going to review the associated NB now 😉

RELEASE-NOTES.md

docs/source/notebooks/GP-Circular.ipynb

twiecki · 2020-09-07T15:19:20Z

Also should be added to the GP kernel NB.

Co-authored-by: Alexandre ANDORRA <[email protected]>

…cular-kernel

ferrine · 2020-09-26T20:24:29Z

I've reviewed the circular kernel once more. In the paper parameterization, the lengthscale parameter appeared to cancel out in computation. I've updated the notebooks and added a comment to the Circular-GP notebook about that

review-notebook-app · 2020-09-27T12:27:33Z

View / edit / reply to this conversation on ReviewNB

AlexAndorra commented on 2020-09-27T12:27:32Z
----------------------------------------------------------------

These lines need to be in a second, separate cell, otherwise the style can have problems (it's because of how matplotlib sets this up):

%config InlineBackend.figure_format = 'retina'
RANDOM_SEED = 8927
np.random.seed(RANDOM_SEED)
az.style.use('arviz-darkgrid')

review-notebook-app · 2020-09-27T12:27:33Z

View / edit / reply to this conversation on ReviewNB

AlexAndorra commented on 2020-09-27T12:27:33Z
----------------------------------------------------------------

"... is proportional to ~~correleation~~ the correlation strength. Let's see by how much!"

review-notebook-app · 2020-09-27T12:27:34Z

View / edit / reply to this conversation on ReviewNB

AlexAndorra commented on 2020-09-27T12:27:34Z
----------------------------------------------------------------

"... the Weinland function..."

review-notebook-app · 2020-09-27T12:27:35Z

View / edit / reply to this conversation on ReviewNB

AlexAndorra commented on 2020-09-27T12:27:34Z
----------------------------------------------------------------

"...an ~~implimentation~~ implementation for the distance..."

review-notebook-app · 2020-09-27T12:27:35Z

View / edit / reply to this conversation on ReviewNB

AlexAndorra commented on 2020-09-27T12:27:35Z
----------------------------------------------------------------

~~What is Weinland function and how it affects the kernel?~~ --> Let's visualize what the Weinland function is, and how it affects the kernel:

review-notebook-app · 2020-09-27T12:27:36Z

View / edit / reply to this conversation on ReviewNB

AlexAndorra commented on 2020-09-27T12:27:36Z
----------------------------------------------------------------

"...let's validate our circular distance function..."

review-notebook-app · 2020-09-27T12:27:36Z

View / edit / reply to this conversation on ReviewNB

AlexAndorra commented on 2020-09-27T12:27:36Z
----------------------------------------------------------------

np.random.seed(42): You don't need to set the seed again, as it is set at the beginning of the NB
plt.figure(figsize=(9, 9)) needs to be outside of the model context
I think the y_sampled aren't used anywhere
Just a question: why is the GP's mean set to 4? I understood it's only tau that needs to be >= 4?

ferrine commented on 2020-10-10T10:45:38Z
----------------------------------------------------------------

y_sampled is used for trace = pm.sample_posterior_predictive([mp], var_names=["y"], samples=100) but the actual variable is recorded my the model context

ferrine commented on 2020-10-10T10:46:18Z
----------------------------------------------------------------

why is the GP's mean set to 4?

It looks better on the plots

review-notebook-app · 2020-09-27T12:27:37Z

View / edit / reply to this conversation on ReviewNB

AlexAndorra commented on 2020-09-27T12:27:37Z
----------------------------------------------------------------

Maybe explicit "RBF", or just say exponential?

AlexAndorra

Thanks @ferrine, really like the new version with the comparisons of different kernels, the take-aways and the explanation 👏
If you don't mind, I think there are still some typos / stuff to explicit, that I flagged above 😉 Tell me if anything is unclear. I think it'll be good to merge after that!

ferrine · 2020-09-29T06:56:11Z

Sure

ferrine · 2020-10-10T10:45:39Z

y_sampled is used for trace = pm.sample_posterior_predictive([mp], var_names=["y"], samples=100) but the actual variable is recorded my the model context

View entire conversation on ReviewNB

ferrine · 2020-10-10T10:46:19Z

why is the GP's mean set to 4?

It looks better on the plots

View entire conversation on ReviewNB

ferrine · 2020-10-10T10:59:34Z

The morning following @AlexAndorra's comments is the best morning

review-notebook-app · 2020-10-10T11:09:22Z

View / edit / reply to this conversation on ReviewNB

MarcoGorelli commented on 2020-10-10T11:09:22Z
----------------------------------------------------------------

Nice notebook!

Small nitpick, but if you could add a trailing semicolon here that'll get rid of Text(0.5, 0, '$x$')

since this is a new notebook, if you could also run

pip install -U nbqa
nbqa black docs/source/notebooks/GP-Circular.ipynb

this would reduce the workload for #4095 :)

AlexAndorra

Amazing, thanks a lot @ferrine 👏 And thanks for bearing with me and all my comments 😄

ferrine added 2 commits September 6, 2020 17:45

Adding Cirdular kernel

8ae98c7

adding an example notebook

9c2dfe8

ad docs, append to release notes

3b24aab

AlexAndorra self-requested a review September 7, 2020 10:09

AlexAndorra added the enhancements label Sep 7, 2020

AlexAndorra requested changes Sep 7, 2020

View reviewed changes

RELEASE-NOTES.md Outdated Show resolved Hide resolved

AlexAndorra reviewed Sep 7, 2020

View reviewed changes

ferrine and others added 6 commits September 7, 2020 21:25

update circular example

8bea0f8

Update RELEASE-NOTES.md

f43bc1a

Co-authored-by: Alexandre ANDORRA <[email protected]>

review nb

a7c8285

Merge branch 'circular-kernel' of github.com:pymc-devs/pymc3 into cir…

c6b9a08

…cular-kernel

fix typos

573b418

circular update only

e9d4f6e

ferrine force-pushed the circular-kernel branch from 262e121 to e9d4f6e Compare September 7, 2020 19:23

ferrine added 2 commits September 7, 2020 22:35

update kernels and covs nb

52af20b

fix typos

430ff1b

ferrine added the WIP label Sep 14, 2020

ferrine added 5 commits September 26, 2020 21:56

bound->period for consistency with Periodic

d2cd80f

update plots, notebooks

eda4b71

fix typo in doc

543f5ea

fix even more typos

3266f46

Merge branch 'master' into circular-kernel

4af6409

ferrine added 2 commits September 26, 2020 23:26

follow pre-commit advice

58e12c8

typo fix

1db3870

AlexAndorra reviewed Sep 27, 2020

View reviewed changes

update notebooks

6ca83c5

ferrine added 2 commits October 10, 2020 15:02

nitpics

cff6775

fix typo

79dbeb6

AlexAndorra approved these changes Oct 10, 2020

View reviewed changes

AlexAndorra merged commit 5e30554 into master Oct 10, 2020

AlexAndorra deleted the circular-kernel branch October 10, 2020 18:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Circular kernel #4082

Circular kernel #4082

ferrine commented Sep 6, 2020 •

edited

Loading

review-notebook-app bot commented Sep 6, 2020

codecov bot commented Sep 6, 2020 •

edited

Loading

AlexAndorra left a comment

twiecki commented Sep 7, 2020

ferrine commented Sep 26, 2020

review-notebook-app bot commented Sep 27, 2020 •

edited

Loading

review-notebook-app bot commented Sep 27, 2020 •

edited

Loading

review-notebook-app bot commented Sep 27, 2020 •

edited

Loading

review-notebook-app bot commented Sep 27, 2020 •

edited

Loading

review-notebook-app bot commented Sep 27, 2020 •

edited

Loading

review-notebook-app bot commented Sep 27, 2020 •

edited

Loading

review-notebook-app bot commented Sep 27, 2020 •

edited

Loading

review-notebook-app bot commented Sep 27, 2020 •

edited

Loading

AlexAndorra left a comment

ferrine commented Sep 29, 2020

ferrine commented Oct 10, 2020

ferrine commented Oct 10, 2020

ferrine commented Oct 10, 2020

review-notebook-app bot commented Oct 10, 2020 •

edited

Loading

AlexAndorra left a comment

Circular kernel #4082

Circular kernel #4082

Conversation

ferrine commented Sep 6, 2020 • edited Loading

review-notebook-app bot commented Sep 6, 2020

codecov bot commented Sep 6, 2020 • edited Loading

Codecov Report

AlexAndorra left a comment

Choose a reason for hiding this comment

twiecki commented Sep 7, 2020

ferrine commented Sep 26, 2020

review-notebook-app bot commented Sep 27, 2020 • edited Loading

review-notebook-app bot commented Sep 27, 2020 • edited Loading

review-notebook-app bot commented Sep 27, 2020 • edited Loading

review-notebook-app bot commented Sep 27, 2020 • edited Loading

review-notebook-app bot commented Sep 27, 2020 • edited Loading

review-notebook-app bot commented Sep 27, 2020 • edited Loading

review-notebook-app bot commented Sep 27, 2020 • edited Loading

review-notebook-app bot commented Sep 27, 2020 • edited Loading

AlexAndorra left a comment

Choose a reason for hiding this comment

ferrine commented Sep 29, 2020

ferrine commented Oct 10, 2020

ferrine commented Oct 10, 2020

ferrine commented Oct 10, 2020

review-notebook-app bot commented Oct 10, 2020 • edited Loading

AlexAndorra left a comment

Choose a reason for hiding this comment

ferrine commented Sep 6, 2020 •

edited

Loading

codecov bot commented Sep 6, 2020 •

edited

Loading

review-notebook-app bot commented Sep 27, 2020 •

edited

Loading

review-notebook-app bot commented Sep 27, 2020 •

edited

Loading

review-notebook-app bot commented Sep 27, 2020 •

edited

Loading

review-notebook-app bot commented Sep 27, 2020 •

edited

Loading

review-notebook-app bot commented Sep 27, 2020 •

edited

Loading

review-notebook-app bot commented Sep 27, 2020 •

edited

Loading

review-notebook-app bot commented Sep 27, 2020 •

edited

Loading

review-notebook-app bot commented Sep 27, 2020 •

edited

Loading

review-notebook-app bot commented Oct 10, 2020 •

edited

Loading