Refactor surrogates and decorators #209
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Hi @AVHopp, @Scienfitz, this PR sits on top of another, so let's wait with the review until the other is merged. |
AdrianSosic
commented
Apr 18, 2024
AdrianSosic
commented
Apr 18, 2024
AdrianSosic
commented
Apr 18, 2024
AdrianSosic
force-pushed
the
refactor/decorators
branch
from
339b15c to
3784326
Compare
|
@AVHopp @Scienfitz: now that #206 is merged, have rebased and is ready for review 🚀 |
AVHopp
reviewed
Apr 24, 2024
Scienfitz
reviewed
Apr 25, 2024
Scienfitz
reviewed
Apr 25, 2024
AdrianSosic
force-pushed
the
refactor/decorators
branch
from
f4d500b to
deb9743
Compare
AVHopp
reviewed
Apr 26, 2024
Scienfitz
added a commit
that referenced
this pull request
Apr 29, 2024
Some progress towards refactoring the acqf interface and enabling more advanced acqfs Done - removed `debotorchize` - incldued some new acqfs, see CHANGELOG - enable iteration tests with all acqfs - extended hypothesis tests Not Done Yet: - removing `AdapterModel` (does not make sense while #209 is open) Issues: - ~~some tests with custom surrogates fail, see separate thread~~ resolved since not using botorch factory anymore - some of the analytical new acqfs dont seem to work wiht out GP model. Eg when I implement the `NEI` I get `botorch.exceptions.errors.UnsupportedError: Only SingleTaskGP models with known observation noise are currently supported for fantasy-based NEI & LogNE`. Also the `LogPI` is available in botorch, but it is not imported to the top level acqf package in botorch, I ignored it here. ~~I included a reverted commit pair so it can be checked out quickly to reproduce~~
AdrianSosic
commented
Apr 30, 2024
AdrianSosic
force-pushed
the
refactor/decorators
branch
from
530f17c to
b3a4572
Compare
Scienfitz
approved these changes
May 2, 2024
AVHopp
approved these changes
May 2, 2024
The old approach was unnecessarily complex and resulted in various problems (i.e. typing issues, duplication of subclasses, etc). The new approach simply exchanges the class' methods (and thus modifies the original class) instead of creating a new one. This requires significantly less and easier code an avoids the aforementioned problems.
The reason is the same as for the refactor of catch_constant_targets in the previous commit.
AdrianSosic
force-pushed
the
refactor/decorators
branch
from
b3a4572 to
bfad013
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR refactors our surrogate package, which significantly removes boilerplate/workaround code and solves several longstanding issues:
catch_constant_targetsandscale_modeldecorators have been drastically reduced/improved. Most importantly, they no longer cause problems in the subclass hierarchy, which allows us to remove many ugly workarounds with serialization hooks, the docs, typing, ...Note: Potentially the
scale_modeldecorator might vanish entirely when refactoring scaling, but this already serves as a preparation as the decorator can now be cleanly removed without other side-effects.The_modelattribute has been pulled up the base class and is now correctly handled in serialization and equality checks.