add docs for interpretation of length scales in periodic kernel

[tirthasheshpatel]

Fixes #3987.

I have added a versionchanged just so that the documentation warns the user when the next version is released. Changed the tests accordingly.

cc @bwengals

[AlexAndorra]

LGTM, thanks @tirthasheshpatel ! I'll wait for @bwengals's approval and merge after that 😉

[bwengals]

I think we ought to consider that this may make peoples past models / inference results wrong, and introduce bugs on the users end, especially since informative priors on lengthscales are a good idea, folks might have an informative prior on ell that would need to be re-scaled. I think here it's not so much a bug, as not using the most common convention. No users code is wrong or broken right now that uses Periodic (at least not because of this!).

So if someone does rerun their model, or are using a prefitted model in production, their new results after this change will have a very subtle bug now, that will be very tough on their end to figure out. I don't think we want this outcome.

Either way, we absolutely need to change the docs / docstring to be correct. But I'm not 100% sure on changing that factor at this point (even if it would have been better from the start).

Thoughts?

[AlexAndorra]

Thanks for the detailed answer @bwengals. I agree with this and I think we have two options:

1. Status-quo: do nothing and not use the standard Periodic kernel formula. This way, there is no backwards issues but it might be confusing to new users: they think we're using the common kernel, set their priors and lenghtscale according to this parametrization and it turns out they should have scaled their priors otherwise (in short, the inverse problem of what you laid out).

2. Changing to the common kernel, with the issues you laid out. I tend to favor this option as it enforces good practices going forward (if I understood correctly what you were saying on the Discourse), which is something we want. From a "rolling out" perspective, this doesn't seem that challenging to me: it's basically akin to a bug whose fixes imply some backwards incompatibility -- but we have experience on how to do that.
If we take this road, maybe we should condition this fix for versions >= 3.9.5 and display a DeprecationWarning during 3.9.3 and 3.9.4.

How does it sound?

[tirthasheshpatel]

Agreed with both of you! What do say about a DeprecationWarning @bwengals. I am cool with keeping it as is or just adding a DeprecationWarning

[junpenglao]

To me it is a breaking change with little gain. I think we should keep it as is and clarify in the comment of the differences in scaling compare to standard/other definitions.

[tirthasheshpatel]

Yeah, I will add a docstring stating the difference between the two conventions.

[bwengals]

Ya agree with @junpenglao, best to just fix the docs so it's clear

[AlexAndorra]

Ok, so let's go with that @tirthasheshpatel ! I think you can do that directly in this PR 😉

[tirthasheshpatel]

Sorry for the late response. Added the docs!

[junpenglao]

Suggested change

	The interpretation of length-scale for this kernel is different than the
	one used normally (see [1]_). Priors on length scale must be put keeping
	this in mind.
	The scaling factor for this kernel is different compare to the other standard
	definition (see [1]_), thus change the interpretation of length-scale in a subtle
	way. Please keep that in mind when you are defining priors on length scale,
	especially when you are trying to replicate other results.
	You can divided the length-scale by 2 in the kernel initialization to recover the
	standard definition.

@bwengals is this correct?

[bwengals]

If I read that it's different, how should one interpret it? I think it'd be good to be specific.

Also small wording suggestion, how does this sound?

Note that the scaling factor for this kernel is different compared to the more common definition (see [1]_). Here, 0.5 is in the exponent instead of the more common value, 4. Divide the length-scale by 2 when initializing the kernel to recover the standard definition.

[tirthasheshpatel]

Sorry for not replying earlier. I missed @bwengals's reply.

Here, 0.5 is in the exponent instead of the more common value, 4.

I think what you meant here was:

Here, 0.5 is in the exponent instead of the more common value, 2.

Right?

[bwengals]

whoops, yep you're right!

[codecov]

Codecov Report

❗ No coverage uploaded for pull request base (master@0790115). Click here to learn what that means.
The diff coverage is n/a.

@@            Coverage Diff            @@
##             master    #3989   +/-   ##
=========================================
  Coverage          ?   86.47%           
=========================================
  Files             ?       88           
  Lines             ?    14090           
  Branches          ?        0           
=========================================
  Hits              ?    12185           
  Misses            ?     1905           
  Partials          ?        0           

Impacted Files	Coverage Δ
pymc3/gp/cov.py	97.96% <ø> (ø)
pymc3/step_methods/__init__.py	100.00% <0.00%> (ø)
pymc3/__init__.py	95.23% <0.00%> (ø)
pymc3/distributions/discrete.py	74.65% <0.00%> (ø)
pymc3/distributions/multivariate.py	78.13% <0.00%> (ø)
pymc3/parallel_sampling.py	88.36% <0.00%> (ø)
pymc3/backends/__init__.py	100.00% <0.00%> (ø)
pymc3/gp/gp.py	92.82% <0.00%> (ø)
pymc3/ode/utils.py	100.00% <0.00%> (ø)
pymc3/model_graph.py	88.48% <0.00%> (ø)
... and 79 more

[bwengals]

good to merge?

[AlexAndorra]

Yep, all good now, thanks @tirthasheshpatel and @bwengals !