Pull out "fantasize" function so it isn't so widely inherited -- for discussion!

[esantorella]

Motivation

The Model base class has a fantasize method, but it isn't actually possible to fantasize from all models. For some models, fantasize fails with a NotImplementedError even though the docstring implies it should work. This is confusing (e.g. #1459 ). Another disadvantage is that codecov doesn't require tests for all of the many fantasize methods that exist, only for the base one -- which can't be called, since it is abstract.

This PR removes the "fantasize" method from the Model base class. Instead, there is a fantasize function that is called by the few classes that do actually use and/or test fantasize.

Pros:

• decreases the "surface area" of BoTorch and prevents misuse of methods that we didn't really intend to exist
• Allows codecov to surface where we aren't actually testing methods (I expect it to fail)

Cons:

• fantasize still exists but doesn't work in HeteroskedasticSingleTaskGP because it inherits from SingleTaskGP (a further refactor can fix that)
• Could remove fantasize from classes where it should exist (despite not being used or tested within BoTorch)
• Somewhat more verbose

Have you read the Contributing Guidelines on pull requests?

Yes

Test Plan

[x] Codecov
[x] Unit tests should pass
[x] No new Pyre errors introduced

[codecov]

Codecov Report

Merging #1462 (4911df3) into main (b103f0a) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##              main     #1462   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files          134       134           
  Lines        12387     12390    +3     
=========================================
+ Hits         12387     12390    +3     

Impacted Files	Coverage Δ
botorch/models/approximate_gp.py	100.00% <ø> (ø)
botorch/models/fully_bayesian.py	100.00% <100.00%> (ø)
botorch/models/fully_bayesian_multitask.py	100.00% <100.00%> (ø)
botorch/models/gp_regression.py	100.00% <100.00%> (ø)
botorch/models/higher_order_gp.py	100.00% <100.00%> (ø)
botorch/models/model.py	100.00% <100.00%> (ø)
botorch/models/model_list_gp_regression.py	100.00% <100.00%> (ø)
botorch/models/pairwise_gp.py	100.00% <100.00%> (ø)
botorch/utils/testing.py	100.00% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[saitcakmak]

I personally like the old pattern a bit better. Having to redefine a fantasize method just to call a common function feels like extra work to me. As mentioned in the internal thread, using a Mixin could make this a bit cleaner.

[saitcakmak]

fantasize still exists but doesn't work in HeteroskedasticSingleTaskGP because it inherits from SingleTaskGP (a further refactor can fix that)

This would still be an issue with the Mixin approach, unless we define a SingleTaskGPBase and add the mixin only to SingleTaskGP and FixedNoiseGP. I think that's too much work for how little it achieves though.

[Balandat]

Another disadvantage is that codecov doesn't require tests for all of the many fantasize methods that exist, only for the base one -- which can't be called, since it is abstract.

Yeah that's an interesting observation. We definitely want to make sure that all methods defined on models are actually tested. I'm not sure though that we need to define the function explicitly on each subclass though - a different solution to this could be to refactor the unit tests for the models into a more generic test harness that enforces testing of all methods defined at the level of the associated model class. What do you think about that approach?

cc @lena-kashtelyan why has been thinking about doing the same for Ax.

[esantorella]

Yeah that's an interesting observation. We definitely want to make sure that all methods defined on models are actually tested. I'm not sure though that we need to define the function explicitly on each subclass though - a different solution to this could be to refactor the unit tests for the models into a more generic test harness that enforces testing of all methods defined at the level of the associated model class. What do you think about that approach?

cc @lena-kashtelyan why has been thinking about doing the same for Ax.

Sounds awesome if there's a good way to do it!

Another option, now that this PR is using a FantasizeMixin, would be to test that the fantasize method works for each non-abstract elementsof FantasizeMixin.__subclasses__. Unfortunately, that isn't an easy test to write because each subclass needs to be instantiated in a different way.

[facebook-github-bot]

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

[facebook-github-bot]

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

[facebook-github-bot]

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