Skip to content

Conversation

@Balandat
Copy link
Contributor

Summary:
In a probit model, the noise is simply a scaling parameter on the function. In the current implementation, the scale is implemented via the noise, which has two disadvantages:

  1. A user coming from anywhere else in gpytorch/botorch wouldn't necessarily know not to use ScaleKernel here.
  2. When running prediction / computing posteriors, by default we don't include the noise (consistent with behavior elsewhere), so our prediction of f(x) is off by an arbitrary constant factor.

This changes the default so that we use ScaleKernel and fix the noise.

Differential Revision: D23854124

@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D23854124

Balandat pushed a commit to Balandat/botorch that referenced this pull request Sep 28, 2020
Summary:
Pull Request resolved: meta-pytorch#571

In a probit model, the noise is simply a scaling parameter on the function. In the current implementation, the scale is implemented via the noise, which has two disadvantages:

1. A user coming from anywhere else in gpytorch/botorch wouldn't necessarily know not to use ScaleKernel here.
2. When running prediction / computing posteriors, by default we don't include the noise (consistent with behavior elsewhere), so our prediction of f(x) is off by an arbitrary constant factor.

This changes the default so that we use ScaleKernel and fix the noise.

Differential Revision: D23854124

fbshipit-source-id: d8ab08264cfc7c09ab027f308ea39921b8147daa
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D23854124

@codecov
Copy link

codecov bot commented Sep 28, 2020

Codecov Report

Merging #571 into master will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##            master      #571   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           84        84           
  Lines         5503      5485   -18     
=========================================
- Hits          5503      5485   -18     
Impacted Files Coverage Δ
botorch/models/pairwise_gp.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1086a38...4c56e7c. Read the comment docs.

…pytorch#571)

Summary:
Pull Request resolved: meta-pytorch#571

In a probit model, the noise is not identifiable separately from a scale on latent function. In the current implementation, this scale is implemented via a noise term, which has three disadvantages:

1. A user coming from anywhere else in gpytorch/botorch wouldn't necessarily know not to use ScaleKernel here.
2. When running prediction / computing posteriors, by default we don't include the noise (consistent with behavior elsewhere), so our prediction of f(x) is off by an arbitrary constant factor.
3. When computing posteriors with `noise=True` the noise is added to the covariance, but this doesn't scale the function value either. This means if I use a batched single-outcome acquisition function to do pairwise acquisition (which is reasonable), I'm not doing acquisition on the same model I'm using for interpolation (since now I have noise on individual items rather than on the comparison). Explicitly pairwise MC acquisition functions that take draws of pairs and do something with them should still be correct here, I think.

This PR changes the default so that we use ScaleKernel and remove the noise term. I've added docs to this effect in a few places, but this is a breaking change API-wise.

Reviewed By: Balandat

Differential Revision: D23854124

fbshipit-source-id: da802d9df05ac1cb873b4f7e4fe6ff9bfa67da8a
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D23854124

@facebook-github-bot
Copy link
Contributor

This pull request has been merged in dd17a4c.

@Balandat Balandat deleted the export-D23854124 branch December 2, 2020 04:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants