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.

Sign up for GitHub

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

Jump to bottom

Get rid of SingleObjectiveBenchmarkProblem #2606

Closed
wants to merge 5 commits into from
Closed

Get rid of SingleObjectiveBenchmarkProblem #2606

wants to merge 5 commits into from

Commits on Jul 28, 2024

  1. Move lazy construction of a surrogate from problem to runner (faceboo… 

    …k#2603)

Summary:
Pull Request resolved: facebook#2603

Context: Surrogate benchmark problems allow for downloading datasets and constructing a surrogate lazily. Since the surrogates and datasets are only needed for the `Runner`, it makes sense to confine this logic to `SurrogateRunner`. This gives surrogate benchmark problems an interface that is much clsoer to that of non-surrogate benchmark problems. In the future, we should be able to get down to just one `BenchmarkProblem` class.

This PR:
* Moves lazy construction of surrogates from the `Problem` to the `Runner`.
* Moves corresponding unit tests from the problem's file to the runner's.
* Removes the attribute `noise_stds` from the problem, since it duplicates the same attribute on the runner and doesn't conform to the interface of other benchmark problems.
* Requires `is_noiseless` to be provided at problem initialization, to make surrogate problems have the same interface as other problems, and adds an attribute `SurrogateRunner.is_noiseless` so that this is not difficult to provide.

Differential Revision: D60266288

Reviewed By: saitcakmak
    @esantorella @facebook-github-bot
    esantorella authored and facebook-github-bot committed Jul 28, 2024
    Configuration menu
    Copy the full SHA
    54a6aef View commit details
    Browse the repository at this point in the history

  2. Require all problems to have an optimal_value; simplify Problem inh… 

    …eritance (facebook#2601)

Summary:
Pull Request resolved: facebook#2601

Context:
* Problems that lack an `optimal_value` get a NaN score and cannot really be used for benchmarking except when aggregated. This is a nasty "gotcha." We don't always know the optimum, but IMO it is better to guess.
* The inheritance structure of benchmark problems is too complex and will make subsequent refactors harder.

This PR:
* Adds an `optimal_value` requirement to `BenchmarkProblem` and makes `BenchmarkProblem` the base class to `SurrogateBenchmarkProblem`, enabling `BenchmarkProblem` to be the only type annotation needed. Therefore, the type annotation `BenchmarkProblemProtocol` is no longer necessary. It will be removed in the next PR.
* No longer allows for giving an NaN score to benchmarks where the problem lacks an optimal value, because this won't happen.
* Updates a lot of annotations.
* Raises an exception for constrained multi-objective problems.
* Use dataclasses to cut down on code.

Differential Revision: D60145193

Reviewed By: saitcakmak
    @esantorella @facebook-github-bot
    esantorella authored and facebook-github-bot committed Jul 28, 2024
    Configuration menu
    Copy the full SHA
    3d6f0e6 View commit details
    Browse the repository at this point in the history

  3. Remove BenchmarkProblemWithKnownOptimum and BenchmarkProtocol typ… 

    …e annotations (facebook#2602)

Summary:
Pull Request resolved: facebook#2602

These are no longer necessary. See previous PR for context. This change reaps them and updates type annotations.

Differential Revision: D60146081

Reviewed By: saitcakmak
    @esantorella @facebook-github-bot
    esantorella authored and facebook-github-bot committed Jul 28, 2024
    Configuration menu
    Copy the full SHA
    aa53efd View commit details
    Browse the repository at this point in the history

  4. Get rid of benchmark problem class constructors (facebook#2605) 

    Summary:
Pull Request resolved: facebook#2605

Context: Benchmark problems are sometimes created with class methods `SingleObjectiveBenchmarkProblem.from_botorch`, `SingleObjectiveBenchmarkProblem.from_botorch_synthetic`, and `MultiObjectiveBenchmarkProblem.from_botorch_multi_objective`, the former two now being identical. This creates the need for some tricky type annotations; to me, replacing these with functions is obviously cleaner. This will make it easier to consolidate classes in the future.

This PR:
* Replaces `SingleObjectiveBenchmarkProblem.from_botorch` and `SingleObjectiveBenchmarkProblem.from_botorch_synthetic` with `create_single_objective_problem_from_botorch`
* Replaces `MultiObjectiveBenchmarkProblem.from_botorch_multi_objective` with `create_multi_objective_problem_from_botorch`

Differential Revision: D60284484

Reviewed By: saitcakmak
    @esantorella @facebook-github-bot
    esantorella authored and facebook-github-bot committed Jul 28, 2024
    Configuration menu
    Copy the full SHA
    79627e7 View commit details
    Browse the repository at this point in the history

  5. Get rid of SingleObjectiveBenchmarkProblem (facebook#2606) 

    Summary:
Pull Request resolved: facebook#2606

The class is not adding anything. We should check if problems are single-objective by looking at the type of their `optimization_config`.

This PR:
* Removes `SingleObjectiveBenchmarkProblem`
* Replaces all references to it with `BenchmarkProblem`
* Removes `get_benchmark_problem`, which is redundant with `get_single_objective_benchmark_problem`

Reviewed By: dme65

Differential Revision: D60285191
    @esantorella @facebook-github-bot
    esantorella authored and facebook-github-bot committed Jul 28, 2024
    Configuration menu
    Copy the full SHA
    646cef4 View commit details
    Browse the repository at this point in the history