refactor: Simplify Prior, Sampler and ConfigEncoder construction
#182
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
The issue stemmed from the fact the `SearchSpace` was putting fidelities in the `.numerical` attribute of the `SearchSpace`, as well as the `.prior` attribute, which didn't really make sense, and caused later use of `space.prior` to not have the kind of information needed. The solution was not to just patch this one issue, but rather remove a lot of the _convenience_ of the `SearchSpace` with the `.prior`, and the `.from_space(..., include_fidelities: bool = ...)` methods, as their behaviour wasn't obvious from construction. Rather now, the `.from_space()` methods have been replaced with `.from_parameters(...)`, where if you'd like to include a fidelitiy in that set of things to operate on, then you can construct it explicitly at the call site and pass it in, rather than `.from_space(space, include_fidelity=False)`. This also allowed us to actually remove a bunch of code and reduce `if` pathways, hopeully reducing the chance of further bugs. ... and yes tests.
Closed
|
Merging, two failed tests are windows, see raised issue #183 |
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.
The issue stemmed from the fact the
SearchSpacewas putting fidelities in the.numericalattribute of theSearchSpace, as well as the.priorattribute, which didn't really make sense, and caused later use ofspace.priorto not have the kind of information needed.The solution was not to just patch this one issue, but rather remove a lot of the convenience of the
SearchSpacewith the.prior, and the.from_space(..., include_fidelities: bool = ...)methods, as their behaviour wasn't obvious from construction. Rather now, the.from_space()methods have been replaced with.from_parameters(...), where if you'd like to include a fidelitiy in that set of things to operate on, then you can construct it explicitly at the call site and pass it in, rather than.from_space(space, include_fidelity=False).This also allowed us to actually remove a bunch of code and reduce
ifpathways, hopeully reducing the chance of further bugs.... and yes tests.
This also revealed an issue I fixed in #181, so I pulled those changes to
SuccessiveHalvingbrackets into here.