Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Circular kernel #4082

Merged
merged 21 commits into from
Oct 10, 2020
Merged

Circular kernel #4082

merged 21 commits into from
Oct 10, 2020

Conversation

ferrine
Copy link
Member

@ferrine ferrine commented Sep 6, 2020

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
Copy link

Check out this pull request on  ReviewNB

Review Jupyter notebook visual diffs & provide feedback on notebooks.


Powered by ReviewNB

@codecov
Copy link

codecov bot commented Sep 6, 2020

Codecov Report

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

Impacted file tree graph

@@            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

Copy link
Contributor

@AlexAndorra AlexAndorra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Outdated Show resolved Hide resolved
docs/source/notebooks/GP-Circular.ipynb Outdated Show resolved Hide resolved
docs/source/notebooks/GP-Circular.ipynb Outdated Show resolved Hide resolved
docs/source/notebooks/GP-Circular.ipynb Outdated Show resolved Hide resolved
docs/source/notebooks/GP-Circular.ipynb Outdated Show resolved Hide resolved
docs/source/notebooks/GP-Circular.ipynb Outdated Show resolved Hide resolved
docs/source/notebooks/GP-Circular.ipynb Outdated Show resolved Hide resolved
docs/source/notebooks/GP-Circular.ipynb Outdated Show resolved Hide resolved
docs/source/notebooks/GP-Circular.ipynb Outdated Show resolved Hide resolved
docs/source/notebooks/GP-Circular.ipynb Outdated Show resolved Hide resolved
docs/source/notebooks/GP-Circular.ipynb Outdated Show resolved Hide resolved
@twiecki
Copy link
Member

twiecki commented Sep 7, 2020

Also should be added to the GP kernel NB.

@ferrine ferrine added the WIP label Sep 14, 2020
@ferrine
Copy link
Member Author

ferrine commented Sep 26, 2020

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
Copy link

review-notebook-app bot commented Sep 27, 2020

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
Copy link

review-notebook-app bot commented Sep 27, 2020

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
Copy link

review-notebook-app bot commented Sep 27, 2020

View / edit / reply to this conversation on ReviewNB

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

"... the Weinland function..."


@review-notebook-app
Copy link

review-notebook-app bot commented Sep 27, 2020

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
Copy link

review-notebook-app bot commented Sep 27, 2020

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
Copy link

review-notebook-app bot commented Sep 27, 2020

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
Copy link

review-notebook-app bot commented Sep 27, 2020

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
Copy link

review-notebook-app bot commented Sep 27, 2020

View / edit / reply to this conversation on ReviewNB

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

Maybe explicit "RBF", or just say exponential?


Copy link
Contributor

@AlexAndorra AlexAndorra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member Author

ferrine commented Sep 29, 2020

Sure

Copy link
Member Author

ferrine commented Oct 10, 2020

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

Copy link
Member Author

ferrine commented Oct 10, 2020

why is the GP's mean set to 4?

It looks better on the plots


View entire conversation on ReviewNB

@ferrine
Copy link
Member Author

ferrine commented Oct 10, 2020

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

@review-notebook-app
Copy link

review-notebook-app bot commented Oct 10, 2020

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 :)


Copy link
Contributor

@AlexAndorra AlexAndorra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@AlexAndorra AlexAndorra merged commit 5e30554 into master Oct 10, 2020
@AlexAndorra AlexAndorra deleted the circular-kernel branch October 10, 2020 18:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants