pymc icon indicating copy to clipboard operation
pymc copied to clipboard

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

Open danhphan opened this issue 1 year ago • 1 comments

Hi, this is not really a bug, but a small change to improve the code consistency in the pymc gp module.

In pymc.gp.gp.py file, several GP classes such as "Marginal" and "MarginalApprox" use noise as a parameter in their marginal_likelihood() function, while other GP classes like "MarginalKron" use sigma as a parameter.

As discussed in https://github.com/pymc-devs/pymc/discussions/6077, it could be better to change the noise parameter to sigma, as well as update documentations to make it clearer.

If anyone is interested in doing this task and need more details, please let me know.

  • PyMC Version: 4 latest

Thank you.

danhphan avatar Sep 02 '22 11:09 danhphan

I also saw this inconsistency in the gp module docs the other day. I agree sigma would be a good consistent solution.

I'd be willing to take a stab at this but @danhphan I see you had some interest.

I'm guessing there would want to be backward compatibility and throw a FutureWarning if using the previous noise parameterization. I'm seeing all the code changes and docstring changes would happen in the pymc/gp/gp.py file.

wd60622 avatar Sep 21 '22 22:09 wd60622

Hi @wd60622 , please go ahead to make a PR for this task.

Yes, most of the code and docstring changes should be in gp.py file, and maybe also in test_gp.py.

Also, a FutureWarning is a good idea! Thank you

danhphan avatar Sep 25 '22 03:09 danhphan

Sounds great. Will open up PR.

I'm first time contributor, but will follow the guidelines

wd60622 avatar Sep 25 '22 11:09 wd60622

I close this issue as @wd60622 fixed it in #6145 .Thanks @wd60622 !

danhphan avatar Dec 14 '22 05:12 danhphan