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

MarginalSparse rename to MarginalApprox #5242

Merged
merged 9 commits into from
Dec 14, 2021

Conversation

bwengals
Copy link
Contributor

@bwengals bwengals commented Dec 6, 2021

Renames gp.MarginalSparse to gp.MarginalApprox as per #5228.

There are also two other related changes that should have been in #5055.

  • cov_func and mean_func weren't forced kwargs in MarginalSparse, fixed that.
  • changed default approx from FITC to VFE. VFE is a better default because it's posterior is a proper GP, while FITC can show heteroskedastic behavior depending on where the inducing point locations are placed. VFE may sometimes overestimate the noise. This follows recommendation of https://proceedings.neurips.cc/paper/2016/file/7250eb93b3c18cc9daa29cf58af7a004-Paper.pdf

add subclass MarginalSparse with warning
change default approx from FITC to VFE
fix bug in whether cov_func and mean_func are forced kwargs
    in MarginalApprox
@twiecki
Copy link
Member

twiecki commented Dec 6, 2021

Should add deprecation warning as well as release notes.

@codecov
Copy link

codecov bot commented Dec 8, 2021

Codecov Report

Merging #5242 (af98fbb) into main (95bd5e5) will increase coverage by 0.23%.
The diff coverage is 77.77%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #5242      +/-   ##
==========================================
+ Coverage   78.94%   79.18%   +0.23%     
==========================================
  Files          88       88              
  Lines       14237    14347     +110     
==========================================
+ Hits        11240    11360     +120     
+ Misses       2997     2987      -10     
Impacted Files Coverage Δ
pymc/gp/gp.py 93.18% <75.00%> (-0.43%) ⬇️
pymc/gp/__init__.py 100.00% <100.00%> (ø)
pymc/backends/report.py 89.51% <0.00%> (-2.10%) ⬇️
pymc/distributions/dist_math.py 86.78% <0.00%> (-0.92%) ⬇️
pymc/model.py 83.26% <0.00%> (-0.05%) ⬇️
pymc/backends/arviz.py 89.51% <0.00%> (-0.05%) ⬇️
pymc/sampling_jax.py 0.00% <0.00%> (ø)
pymc/initial_point.py 100.00% <0.00%> (ø)
pymc/distributions/bound.py 100.00% <0.00%> (ø)
pymc/distributions/discrete.py 98.72% <0.00%> (+0.36%) ⬆️
... and 5 more

@bwengals bwengals changed the title Marginalsparse rename MarginalSparse rename Dec 8, 2021
@bwengals bwengals changed the title MarginalSparse rename MarginalSparse rename to MarginalApprox Dec 8, 2021
@twiecki
Copy link
Member

twiecki commented Dec 8, 2021

LGTM. Are the CI problems related to this?

@bwengals
Copy link
Contributor Author

Yes, tests failing was my bad. Passing now.

@twiecki twiecki merged commit 1e323be into pymc-devs:main Dec 14, 2021
@bwengals bwengals deleted the marginalsparse-rename branch December 16, 2021 00:55
@michaelosthege michaelosthege added the GP Gaussian Process label Jan 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
GP Gaussian Process
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants