Gp sigma parameterize 6094 #6145

williambdean · 2022-09-25T13:12:05Z

What is this PR about?
Addressing the issue raised: #6094

Helps migrates toward standardized API for noise in GPs. Changes the noise parameter to sigma where applicable. A FutureWarning is used if noise parameter is still used in the previous locations.

Checklist

Explain important implementation details 👆
Make sure that the pre-commit linting/style checks pass.
Link relevant issues (preferably in nice commit messages)
Are the changes covered by tests and docstrings?
Fill out the short summary sections 👇

Major / Breaking Changes

NA

Bugfixes / New features

Standardize the gp API

Docs / Maintenance

GP docstring examples all work and use sigma parameter instead of noise
Small changes to the Latent GP docs that mistakenly reference the noise

williambdean · 2022-09-25T13:27:09Z

Previous tests work on my end with the expect warnings. Working through specific tests soon for warnings

…tests

codecov · 2022-09-25T15:20:54Z

Codecov Report

Merging #6145 (318e345) into main (1417785) will increase coverage by 1.09%.
The diff coverage is 98.80%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6145      +/-   ##
==========================================
+ Coverage   91.93%   93.02%   +1.09%     
==========================================
  Files         101       91      -10     
  Lines       20948    20752     -196     
==========================================
+ Hits        19258    19305      +47     
+ Misses       1690     1447     -243

Impacted Files	Coverage Δ
pymc/aesaraf.py	`93.38% <ø> (ø)`
pymc/distributions/transforms.py	`100.00% <ø> (ø)`
pymc/model.py	`88.23% <ø> (ø)`
pymc/gp/gp.py	`93.82% <96.55%> (+0.62%)`	⬆️
pymc/distributions/continuous.py	`97.50% <100.00%> (ø)`
pymc/distributions/multivariate.py	`92.31% <100.00%> (ø)`
pymc/smc/smc.py	`97.24% <100.00%> (ø)`
pymc/tests/gp/test_gp.py	`100.00% <100.00%> (ø)`
pymc/variational/updates.py	`92.11% <100.00%> (ø)`
pymc/parallel_sampling.py	`85.80% <0.00%> (-1.00%)`	⬇️
... and 12 more

williambdean · 2022-09-25T22:45:48Z

The implementation should be at the point initially described. Namely,

switch logic and docs to use sigma parameter
allow for noise parameter until more formally deprecated. Have a warning to indicate

Think the tests are in a fine place as well. Was thinking of having one to compare the model parameters with the noise vs sigma parameterization but might be overkill way to test handling the parameters. Could just test function if desired.
Also, put the tests into a separate spot so they can be removed at some point.

Noticed a failing test on one of the windows runs from before. Don't know too much about that at the moment. It doesn't seem related to the changes I've made at first glance. Not sure if anyone has thoughts here.

pymc/gp/gp.py

bwengals · 2022-09-26T20:06:40Z

Thanks for the contribution! I think from a user standpoint, it makes sense to have all functions take sigma.

For some background, the previous reason why one GP implementation was noise and the other was sigma was because in gp.Marginal you can pass in any covariance function for the noise, white $\sigma^2 \mathbf{I}$ or otherwise. But gp.MarginalApprox only handles white noise, so it's input was specifically $\sigma$ and not noise more generally.

That said, while it makes sense to have more simplicity for users to input sigma (and say the type can be an arbitrary covariance matrix) it is somewhat incorrect to have things like Knx = sigma(X) in the code, since one would reasonably assume sigma was a scalar and not an arbitrary covariance function. What about changing the name to noise_func (or similar) to go with cov_func internally?

danhphan · 2022-09-27T10:14:10Z

Hi @wd60622 Thank you for the PR, and thanks @bwengals for the comments.

I will have a look in more details at this on tomorrow afternoon. Also, sorry for my late response!

williambdean · 2022-09-27T12:47:38Z

Great. Thank you both for taking a look.
I like your suggestions @bwengals. Agree with the noise_func internal. Good suggestion for clarity.

I will make the adjustments a bit later today

williambdean · 2022-09-27T22:18:07Z

@bwengals Changes implemented. Went with noise_func alternative. I like that suggestion and added wherever the previous sigma was being called. That transformed noise_func is still being saved in the sigma attribute as that is the argument and the name in the decorator. Tried to stay as true to the previous logic as possible. But feels a little weird to me as it is transformed. Let me know if you have any other comments on this implementation.

bwengals · 2022-09-28T00:36:16Z

This looks great! For the decorator, for using .conditional with given, the user still specifies it as sigma right? I think as long as the user doesn't see or interact with noise_func it works well.

williambdean · 2022-09-28T00:55:12Z

This looks great! For the decorator, for using .conditional with given, the user still specifies it as sigma right? I think as long as the user doesn't see or interact with noise_func it works well.

Yup, correct. Still sigma being passed / is the main attribute being used so the decorator is appropriate.
Just my thought there is that sigma will be transformed into Covariance (WhiteNoise) instance and being passed around under the name noise_func even though it is stored in self.sigma.
But a user could pass an instance of that class in the first place so I guess it is not that weird to do. Bit thinking out loud here 😄

Just waiting on these tests and will decode any fails when they are done. They have been successful in my the local checks.

danhphan

Hi, thanks @wd60622

Look good to me. I just add a couple of small comments for clarification.

danhphan · 2022-09-28T01:55:15Z

pymc/gp/gp.py

+    if (sigma is None and noise is None) or (sigma is not None and noise is not None):
+        raise ValueError("'sigma' argument must be specified.")


Hi, just want to understand the logic: If sigma is not None and noise is not None, should we raise this error, as we already specified sigma in this case?

Maybe, the logic here could be:

if `sigma is not None`: good else: if `noise is not None`: raise warning else: raise error.

OR

if `sigma is None`: if `noise is None`: raise error else: raise warning

If both sigma and noise are specified, it is not really wrong, right? :) The user just provides more inputs than what is needed.

I hear what you are staying but thought it might be fine to nudge them toward the new input. Not likely for both to be used, but thought being explicit about the intended behavior would be best instead of throwing away or checking the two parameters silently.

If the user passes in sigma=0.1 and noise=0.2, it's not really possible from the outside to know which value is going to be used in the model. The user must be a bit confused then, and an error message would help them out. Both your alternatives for doing the logic make sense to me too though @danhphan, but I think it also makes sense how it is.

Well said @bwengals That's what I had in mind.
Was trying to make it easier from the developer's point of view

Hi, thanks @wd60622 and @bwengals for the clarification. Total agree with "nudge users toward the new sigma" :)

danhphan · 2022-09-28T02:18:45Z

pymc/gp/gp.py

            Function input values.
        given: dict
-            Can optionally take as key value pairs: `X`, `y`, `noise`,
+            Can optionally take as key value pairs: `X`, `y`,


Do we need to replace noise by sigma here?

This was for the latent class that doesn't use noise. I believe this was a mistake to begin with. Let me know if I am wrong

Yes @wd60622 you are right! thanks to you both for catching this I didn't notice it....

Great, thanks for pointing it out :)

danhphan

Many thanks @wd60622 for the contribution!

bwengals

LGTM

williambdean added 2 commits September 25, 2022 09:01

Standardizing 'noise' parameter to be called 'sigma'

3df0d4a

reformatting with black / pre-commit

c1e1500

removing noise param in tests / removing related futurewarnings from …

1293148

…tests

looking for the warnings and the value error

13e444d

williambdean marked this pull request as ready for review September 25, 2022 22:38

bwengals reviewed Sep 26, 2022

View reviewed changes

pymc/gp/gp.py Outdated Show resolved Hide resolved

capitalize acronyms and "noise_func" instead of "sigma"

318e345

danhphan reviewed Sep 28, 2022

View reviewed changes

danhphan approved these changes Sep 28, 2022

View reviewed changes

bwengals approved these changes Sep 29, 2022

View reviewed changes

canyon289 merged commit e07eea7 into pymc-devs:main Sep 29, 2022

williambdean deleted the gp_sigma_parameterize_6094 branch September 29, 2022 15:56

danhphan mentioned this pull request Dec 14, 2022

Use only sigma as a parameter in gp.marginal_likelihood() #6094

Closed

williambdean mentioned this pull request Aug 1, 2025

Remove deprecated noise parameter for GPs #7886

Merged

11 tasks

		if (sigma is None and noise is None) or (sigma is not None and noise is not None):
		raise ValueError("'sigma' argument must be specified.")

Gp sigma parameterize 6094 #6145

Gp sigma parameterize 6094 #6145

Uh oh!

Conversation

williambdean commented Sep 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Major / Breaking Changes

Bugfixes / New features

Docs / Maintenance

Uh oh!

williambdean commented Sep 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Sep 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

williambdean commented Sep 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

bwengals commented Sep 26, 2022

Uh oh!

danhphan commented Sep 27, 2022

Uh oh!

williambdean commented Sep 27, 2022

Uh oh!

williambdean commented Sep 27, 2022

Uh oh!

bwengals commented Sep 28, 2022

Uh oh!

williambdean commented Sep 28, 2022

Uh oh!

danhphan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

danhphan left a comment

Choose a reason for hiding this comment

Uh oh!

bwengals left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

williambdean commented Sep 25, 2022 •

edited

Loading

williambdean commented Sep 25, 2022 •

edited

Loading

codecov bot commented Sep 25, 2022 •

edited

Loading

williambdean commented Sep 25, 2022 •

edited

Loading