Make (Log)NoisyExpectedImprovement create a correct fantasy model with non-default SingleTaskGP #2414

71c · 2024-07-08T23:27:57Z

Motivation

In botorch/acquisition/analytic.py, the LogNoisyExpectedImprovement and NoisyExpectedImprovement use the function _get_noiseless_fantasy_model in order to repeatedly sample from fantasy model. But _get_noiseless_fantasy_model only works for default GP (i.e. with default Matern kernel) & also with no input or outcome transforms.
I think that it would make sense if this code were written to work with any kind of SingleTaskGP, not just the default one with no input and outcome transforms.

Have you read the Contributing Guidelines on pull requests?

Yes

Test Plan

Since the code is now meant to work even if there are input or outcome transforms or different covar_module or mean_module, I updated the test code to try all these things, as well as try different input bounds to make sure the input transform is working correctly. However, the tests now fail, specifically when either the input data range is not [0,1], or when the kernel is the RBF kernel (not Matern). I believe that the tests failing when RBF is used is simply because certain constants were used in the code that are only valid for particular GP settings. However, I think that when the code fails due to input range not being [0,1], this might be a slight problem with the code -- or might not -- I'm not completely sure.

I also added a line that makes sure that the state_dict() is the same between the original model and fantasy model.

Related PRs

I put this in an issue #2412 and was told that it is OK if not all the tests pass.

esantorella

Thanks so much for reporting this issue and for doing so much of the work towards fixing it! I'm not sure the handling of transforms is right -- see comments. And if you don't have time to get this PR past the finish line yourself, just let us know.

71c · 2024-07-11T01:20:55Z

Thanks so much for reporting this issue and for doing so much of the work towards fixing it! I'm not sure the handling of transforms is right -- see comments. And if you don't have time to get this PR past the finish line yourself, just let us know.

Hi thanks for reviewing my pull request. I don't know what you mean by "see comments" - I don't see any comments.
And regarding whether I can finish it myself, I see that the tests were hard-coded, but I'm not sure how the person who wrote those tests came up with those constants...so I'm not sure how to adapt these test to test this where there could be input and outcome transforms and different kernel.

I think that I won't have the time or ability to finish this any time in the near future.
But one thing is for sure, which is that what I wrote is better than what there was previously...it's just that, of course, ideally we will make sure that the code is completely correct before letting it go through.

botorch/acquisition/analytic.py

esantorella · 2024-07-11T17:52:41Z

Ah, I'm sorry, I failed to submit my comments yesterday. No worries if you don't have time to work on this; I just wanted to check on what you intentions are.

I think we'd want to test combining transforms with (Log)NEI by comparing against the behavior without transforms. For example, if we evaluate LogNEI on a train_X that has already been normalized to [0, 1] and don't use transforms, we should get the same acquisition values if we add a constant to train_X and apply the Normalize input transform.

71c · 2024-07-11T22:02:58Z

I fixed the problems now and all the tests pass. So I think it is ready to be committed to main.
However, I see that Sebastian just made a significant change to test_analytic.py... I'll update my code for that.

… conflicts

71c · 2024-07-11T22:36:04Z

@esantorella There's still conflicts. I'm not sure how to merge them correctly though (either through GitHub or locally on command line). Any help would be appreciated!

SebastianAment · 2024-07-12T14:02:01Z

test/acquisition/test_analytic.py

+        # Same as the default Matern kernel
+        # botorch.models.utils.gpytorch_modules.get_matern_kernel_with_gamma_prior,
+        # except RBFKernel is used instead of MaternKernel.
+        # For some reason, RBF gives numerical problems but Matern does not.


Which numerical problems did you run into? The RBF kernel is generally smoother as the Matern kernel, meaning its eigen-spectrum decays faster, which implies that associated covariance matrices are more likely to be numerically low rank than for the Matern. Evaluating the kernel on fewer points, or alternatively decreasing its lengthscale should make this type of numerical issue go away.

You mean increasing the lengthscale right? But in any case, I think that would be too much of a pain because firstly, NEI_NOISE is fixed at 10 values so it's not straightforward to decrease the number of points. And secondly, the lengthscale really matters for making the tests work; whoever made this test I assume knew what they were doing and I don't want to arbitrarily fudge numbers (namely, the lengthscale) too much because I saw that that can make the tests fail. Why not just keep it how it is?

How about this, if we really don't want those numerical warnings: test Matern on both float32 and float64, but only test RBF on float64?

You mean increasing the lengthscale right?

Increasing the lengthscale will tend to exacerbate the numerical ill conditioning of the RBF kernel matrix, decreasing the lengthscale will help it. You can think of the lengthscale as controlling the potential complexity of the function, and functions with larger lengthscales are smoother, i.e. less complex, than functions with shorter lengthscales, which can exhibit many more variations. This in turn shows up as an increase in the numerical rank for more complex functions, which helps the conditioning.

In the most extreme case, a lengthscale of zero would imply that all points are independent, i.e. the covariance would be diagonal, and in the case of the unscaled RBF kernel, it'd be the identity, which has perfect conditioning.

but only test RBF on float64

Sure!

test/acquisition/test_analytic.py

…format code

codecov · 2024-07-12T18:27:08Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.98%. Comparing base (6892be9) to head (26066d3).
Report is 3 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #2414   +/-   ##
=======================================
  Coverage   99.98%   99.98%           
=======================================
  Files         189      189           
  Lines       16685    16691    +6     
=======================================
+ Hits        16683    16689    +6     
  Misses          2        2

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

facebook-github-bot · 2024-07-12T18:34:29Z

@SebastianAment has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

SebastianAment · 2024-07-12T18:41:11Z

This looks good to me, thanks! I just imported the PR, which will run some additional integration tests. Two remaining items to get this in:

@esantorella could you approve the changes if they were addressed?
@71c we can't merge the PR into the main branch while it's still in draft. Could you put it out of draft mode?

71c · 2024-07-12T19:07:47Z

@71c we can't merge the PR into the main branch while it's still in draft. Could you put it out of draft mode?

@SebastianAment This PR used to be in draft mode but I don't see anything on this page about it being in draft mode; it looks to me like it's not in draft mode anymore.

SebastianAment · 2024-07-12T19:37:13Z

Yes same here, maybe it was changed automatically by the import.

botorch/acquisition/analytic.py

esantorella · 2024-07-15T19:00:57Z

botorch/acquisition/analytic.py

+
+    # Could pass in the outcome_transform and input_transform here,
+    # however that would make them be applied in SingleTaskGP.__init__ which is
+    # unnecessary. So we will instead set them afterwards.
    fantasy_model = SingleTaskGP(


This change would allow for supporting more model types. However, @saitcakmak had a good question: Why do we need _get_noiseless_fantasy_model at all? Can we use the fantasize method on the model instead? I'm a bit afraid of the change I'm suggesting, since this instantiation logic won't be right for every model.

Suggested change

fantasy_model = SingleTaskGP(

fantasy_model = cls(model)(

Yeah, it'd be a great simplification (& removal of duplicate logic) if we can simply use model.fantasize(...) rather than defining a custom _get_noiseless_fantasy_model.

I think you mean to write model.__class__(...) or type(model)(...); cls(model) is not a thing in Python.
But anyway, how about just not making that change because the model is currently assumed to be a SingleTaskGP anyway, and if we can find a way to use fantasize for a wider variety of models later, then just do that.

esantorella · 2024-07-15T20:41:32Z

Thanks! This looks good to me. I checked the acquisition values with and without having transforms on the model here and see that the transforms are being handled correctly -- you can't see all the (Log)NEI lines here because they're all on top of each other. And they're all reasonably close to qLogNEI, which hasn't changed.

One thing I don't quite understand is why fantasization needs to happen this way instead of using the model's fantasize method. It's strange that there's this ad-hoc logic for just NEI and LogNEI. It would be great if the fantasization could be handled by the model, because then we'd accommodate a broader set of model classes, as well as models without observed noise.

esantorella · 2024-07-15T20:42:37Z

botorch/acquisition/analytic.py

@@ -608,7 +607,7 @@ class LogNoisyExpectedImprovement(AnalyticAcquisitionFunction):

    def __init__(
        self,
-        model: GPyTorchModel,
+        model: SingleTaskGP,


Although it's a very ugly hack, I'd suggest leaving this as GPyTorchModel and raising an UnsupportedError when the model is something other than a SingleTaskGP, because this will create a lot of downstream typecheck errors.

OK yeah, that's what's currently done in _get_noiseless_fantasy_model. Which is, like you say, hacky since it says that the type is GPyTorchModel.

This makes me think of another thing, which is, SingleTaskGP can be multiple-outcome, but in LogNoisyExpectedImprovement and NoisyExpectedImprovement it is assumed to be single-outcome, but there is no explicit check for this.

But then, there's the fact that all the other classes in analytic.py, and probably most of the BoTorch code, it looks like doesn't explicitly check that the models are single-outcome, even if it says in the docstring that it should be. I guess with some coding styles then you don't always do explicit checks in the code, so it's not like this is a necessity...

…odel parameter type clearer in description of model parameter in addition to the current note; added explicit check to make sure that the model is single-outcome

Co-authored-by: Elizabeth Santorella <elizabeth.santorella@gmail.com>

facebook-github-bot · 2024-07-22T15:59:34Z

@SebastianAment has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

facebook-github-bot · 2024-07-22T18:12:18Z

@SebastianAment merged this pull request in 25506ab.

71c added 2 commits July 7, 2024 00:27

broken code

cf2f4d8

finalized change

7a878ad

facebook-github-bot added the CLA Signed Do not delete this pull request or issue due to inactivity. label Jul 8, 2024

esantorella requested changes Jul 10, 2024

View reviewed changes

esantorella reviewed Jul 11, 2024

View reviewed changes

botorch/acquisition/analytic.py Outdated Show resolved Hide resolved

Fixed tests and improved code -- all tests pass now

454e884

Try to make the code more similar to Sebastian's commit so there's no…

5f28d39

… conflicts

SebastianAment reviewed Jul 12, 2024

View reviewed changes

Merge branch 'main' into improve_model_data_api

3a55eaf

SebastianAment requested changes Jul 12, 2024

View reviewed changes

test/acquisition/test_analytic.py Outdated Show resolved Hide resolved

SebastianAment reviewed Jul 12, 2024

View reviewed changes

test/acquisition/test_analytic.py Outdated Show resolved Hide resolved

Skip tests with RBF + float32 because those have numerical problems; …

e2d5da5

…format code

SebastianAment marked this pull request as ready for review July 12, 2024 18:37

SebastianAment approved these changes Jul 12, 2024

View reviewed changes

esantorella reviewed Jul 15, 2024

View reviewed changes

botorch/acquisition/analytic.py Outdated Show resolved Hide resolved

esantorella reviewed Jul 15, 2024

View reviewed changes

71c and others added 2 commits July 16, 2024 00:07

Reverted model type annotation back to GPyTorchModel; made expected m…

31f2a80

…odel parameter type clearer in description of model parameter in addition to the current note; added explicit check to make sure that the model is single-outcome

Update comment in _get_noiseless_fantasy_model about transforms

a577213

Co-authored-by: Elizabeth Santorella <elizabeth.santorella@gmail.com>

saitcakmak approved these changes Jul 18, 2024

View reviewed changes

saitcakmak added 3 commits July 18, 2024 11:44

misc changes to comments

85033a4

Patch codecov

a0c67f4

lint

26066d3

facebook-github-bot closed this in 25506ab Jul 22, 2024

facebook-github-bot added the Merged label Jul 22, 2024

saitcakmak mentioned this pull request Jul 24, 2024

[Bug] _get_noiseless_fantasy_model for LogNoisyExpectedImprovement and NoisyExpectedImprovement only works for default GP with default Matern & no input or outcome transforms #2412

Closed

Make (Log)NoisyExpectedImprovement create a correct fantasy model with non-default SingleTaskGP #2414

Make (Log)NoisyExpectedImprovement create a correct fantasy model with non-default SingleTaskGP #2414

Uh oh!

Conversation

71c commented Jul 8, 2024

Motivation

Have you read the Contributing Guidelines on pull requests?

Test Plan

Related PRs

Uh oh!

esantorella left a comment

Choose a reason for hiding this comment

Uh oh!

71c commented Jul 11, 2024

Uh oh!

Uh oh!

esantorella commented Jul 11, 2024

Uh oh!

71c commented Jul 11, 2024

Uh oh!

71c commented Jul 11, 2024

Uh oh!

SebastianAment Jul 12, 2024

Choose a reason for hiding this comment

Uh oh!

71c Jul 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SebastianAment Jul 12, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

codecov bot commented Jul 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

facebook-github-bot commented Jul 12, 2024

Uh oh!

SebastianAment commented Jul 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

71c commented Jul 12, 2024

Uh oh!

SebastianAment commented Jul 12, 2024

Uh oh!

Uh oh!

esantorella Jul 15, 2024

Choose a reason for hiding this comment

Uh oh!

saitcakmak Jul 15, 2024

Choose a reason for hiding this comment

Uh oh!

71c Jul 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

esantorella commented Jul 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

esantorella Jul 15, 2024 • edited by saitcakmak Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

71c Jul 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

71c Jul 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot commented Jul 22, 2024

Uh oh!

facebook-github-bot commented Jul 22, 2024

Uh oh!

Uh oh!

71c Jul 12, 2024 •

edited

Loading

codecov bot commented Jul 12, 2024 •

edited

Loading

SebastianAment commented Jul 12, 2024 •

edited

Loading

71c Jul 16, 2024 •

edited

Loading

esantorella commented Jul 15, 2024 •

edited

Loading

esantorella Jul 15, 2024 •

edited by saitcakmak

Loading

71c Jul 16, 2024 •

edited

Loading

71c Jul 16, 2024 •

edited

Loading