Support different noise levels for different outputs in test functions

[seanohagan]

Adds support for different noise levels for different objectives in a multiobjective test function

Closes #2135

Modifies BaseTestProblem class to allow for noise_std to be a list of floats, where the length should be the number of objectives in a MultiObjectiveTestProblem. This way each component of the objective function can be subject to a separate noise level.

[Balandat]

Thanks a lot for getting started on this!

Overall this looks great, a couple of minor inline comments. Could you please also update the docstrings to explain how the new list arg works?

The other thing that needs updating are the functions in https://github.com/pytorch/botorch/blob/main/botorch/test_functions/synthetic.py - this should largely be a search and replace exercise. There are also constrained problems (subclassing also from ConstrainedBaseTestProblem) that would currently not be handled by these changes - if you feel up for it you can think through what a good setup / interface for that could look like. But that's also something that you (or someone else) could do in a follow-up PR.

[facebook-github-bot]

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

[seanohagan]

Thank you for the helpful feedback! I've implemented almost all of your suggestions (updating type hints to more readable format, updating docstrings, changing to torch.tensor with device, adding a verification for the length of the list and a test for it).

A question regarding this remark:
"The other thing that needs updating are the functions in https://github.com/pytorch/botorch/blob/main/botorch/test_functions/synthetic.py - this should largely be a search and replace exercise"

For the SyntheticTestFunction base class, I've updated the type hints and docstring, but it seems like all of its child classes (in test_functions/synthetic.py and test_functions/multi_fidelity.py are real-valued/single-objective examples. I wonder if the type hints (and docstrings) should remained unchanged as noise_std: Optional[float] for single-objective test problems (since a list should not be given for a problem with a single objective).

[Balandat]

Thanks for the updates.

For the SyntheticTestFunction base class, I've updated the type hints and docstring, but it seems like all of its child classes (in test_functions/synthetic.py and test_functions/multi_fidelity.py are real-valued/single-objective examples. I wonder if the type hints (and docstrings) should remained unchanged as noise_std: Optional[float] for single-objective test problems (since a list should not be given for a problem with a single objective).

Yes, I think that makes a lot of sense. The constrained problems at the end of synthetic.py technically do have multiple outputs, but this is exposed through the evaluate_slack function, so for these we do need to think of a separate way of defining the noise level - right now this is done here assuming that noise_std is a scalar. There currently isn't a separation of the noise level between objective(s) and constraints. I think the cleanest way to do this would be to give ConstrainedBaseTestProblem a separate list of constraint noise levels, say constraint_noise_std: Union[None, float, List[float]] that could be used in a similar way. In order to expose this cleanly, we probably have a few options:

1. Define a constructor for each synthetic constrained problem subclassing both SyntheticTestFunction and ConstrainedBaseTestProblem to ensure that constraint_noise_std can be set and assigned as an attribute
2. Define a ConstrainedSyntheticTestFunction that defines a generic constructor that basically just calls SyntheticTestFunction.__init__() and then assigns the constraint_noise_std attribute. Then all constrained synthetic problems can just subclass that and we don't have to write multiple constructors.

[seanohagan]

Thanks for the suggestions. I liked the approach that you described

I gave the base class a setup_constraint_noise method and called it from the constructor instead of putting this logic in the constructor solely to reduce code duplication in ConstrainedHartmann and ConstrainedHartmannSmooth-- I figured I either had to write constructors for these two and do it like this, or basically just copy the Hartmann logic into them, and I preferred this.

[facebook-github-bot]

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

[Balandat]

Very nice, thanks for whipping this into shape so quickly.

FYI there is a some internal code that we will have to update based on these changes, so while this is pretty much ready to go in of itself (modulo the minor changes I suggested), it may be a few days until we can actually merge this in so as to not break anything downstream.

Thanks a lot for your contribution!

[facebook-github-bot]

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

[facebook-github-bot]

@Balandat merged this pull request in 449b911.