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

Expose jitter #2136

Merged
merged 13 commits into from
Sep 15, 2022
Merged

Expose jitter #2136

merged 13 commits into from
Sep 15, 2022

Conversation

hughsalimbeni
Copy link
Contributor

  • Exposes jitter in variational_strategy
  • Added a test that checks variational vs exact gp with high precision comparison. This is not possible without setting jitter to a very small value

One small issue is that the previous behaviour had both 1e-3 and 1e-4 added in various places. I've only found one test that breaks with 1e-4 for both, so I've set this one to 1e-3 via the new exposed jitter_val arg.

Happy to do the other variational strategies, and move the defaults to the settings.

Copy link
Member

@jacobrgardner jacobrgardner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm other than the lint errors and the import question!

@gpleiss
Copy link
Member

gpleiss commented Sep 14, 2022

Thanks for this @hughsalimbeni ! If you wouldn't mind also making these changes to the other variational strategies that would be great!

@hughsalimbeni
Copy link
Contributor Author

In the last commit I found one more in the grid interpolation strategy, but this time it breaks with anything smaller than 1e-3, so I've left explicit for now.

The default is now changed from 1e-3 to 1e-6 (or 1e-8 for float64), which is quite a large change. Might this cause problems for some users?

@gpleiss
Copy link
Member

gpleiss commented Sep 14, 2022

The default is now changed from 1e-3 to 1e-6 (or 1e-8 for float64), which is quite a large change. Might this cause problems for some users?

I imagine this should be okay - we had 1e-3 or 1e-4 in our ExactGP code until we moved to the context manager, and (AFAIK) it hasn't introduced too many numerical issues. We can always tighten this (or make a separate setting for variational models) if we see trouble down the line).

Thanks @hughsalimbeni !

@gpleiss gpleiss enabled auto-merge (squash) September 14, 2022 23:48
auto-merge was automatically disabled September 15, 2022 07:48

Head branch was pushed to by a user without write access

@hughsalimbeni
Copy link
Contributor Author

Odd that the test_decoupled_svgp_regression test failed as it was fine on my machine. I've changed to 1e-4. Hope it is ok now

@jacobrgardner
Copy link
Member

Interesting also that it's failing against pytorch master only, but not pytorch's current stable release. Maybe 1e-6 is problematically small for variational GPs in fp32 and we should make a settiings.variational_cholesky_jitter with a default of 1e-4? For what it's worth, I've empirically noticed that K_uu matrices can be pretty artificially numerically sensitive with learned inducing locations, since optimization can sometimes put a few inducing points essentially directly on top of each other.

@gpleiss
Copy link
Member

gpleiss commented Sep 15, 2022

I'm going to add that setting, and then merge.

@gpleiss gpleiss enabled auto-merge (squash) September 15, 2022 14:19
@gpleiss gpleiss merged commit 8b2d304 into cornellius-gp:master Sep 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants