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

GP Periodic Kernel uses uncommon formula #3987

Closed
AlexAndorra opened this issue Jul 1, 2020 · 3 comments · Fixed by #3989
Closed

GP Periodic Kernel uses uncommon formula #3987

AlexAndorra opened this issue Jul 1, 2020 · 3 comments · Fixed by #3989
Labels

Comments

@AlexAndorra
Copy link
Contributor

As discussed on the Discourse, PyMC3's Periodic kernel is implemented as:

equation

But, in other sources such as David Duvenaud’s Kernel Cookbook, as well as the NB for mean and cov functions, the formula is given by:
equation

According to @bwengals, this changes the interpretation of the lengthscale: a constant factor changes how you think of the lengthscale and what priors you’d set on it.
So, ideally, PyMC3's implementation should be changed to the second equation above -- note that this would not be backwards-compatible then, so it should be accompanied by a warning. Does anybody disagree?

A final note: it seems like we’re using another slightly different implementation in PyMC4:

equation
@tirthasheshpatel do you confirm it is equivalent to the standard formula? I notice there is a new sigma ** 2 parameter and the distance between x and x' is squared 🤔

@AlexAndorra AlexAndorra added the bug label Jul 1, 2020
@tirthasheshpatel
Copy link
Contributor

tirthasheshpatel commented Jul 1, 2020

equation
@tirthasheshpatel do you confirm it is equivalent to the standard formula? I notice there is a new sigma ** 2 parameter and the distance between x and x' is squared 🤔

Where did you see this equation in PyMC4? Is it present in the docs or notebooks? Actually PyMC4 doesn't use this. Instead, it uses the implementation from the kernel cookbook. And yes, the sigma ** 2 is the amplitude which was implemented in PyMC3 by externally multiplying the kernel with it.

@tirthasheshpatel
Copy link
Contributor

Oh sorry! It is present in the notebooks. I will change that. Thanks for noticing!

@AlexAndorra
Copy link
Contributor Author

Yes, it was in the NB with all the cov functions, in your PR.
FWIW, I find it better to integrate to amplitude parameter into the kernel formula, as we do in PyMC4 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants