Multi-fidelity surrogate models - #721
Conversation
d062293 to
8ed0f2a
Compare
ccac1f8 to
178772a
Compare
AVHopp
left a comment
There was a problem hiding this comment.
Some comments on everything but the multi_fidelity.py file, as I guess that we will discuss this today in a bit more detail.
2e64082 to
8e8547b
Compare
|
Hey @jpenn2023, I just wanted to have a look at your PR but then noticed that something went wrong with your rebase. I know you asked me about the weird diff shown on Github (and yes, that is generally still a problem), but it turns out that your diff is wrong for a different reason – namely because you somehow rebased your commits together with an entire bunch of other commits 😬 Can you quickly fix it and ping me once ready for review? |
d96cf75 to
2c513b0
Compare
2c513b0 to
754d5da
Compare
754d5da to
563b86a
Compare
563b86a to
88db97f
Compare
|
Hey @jpenn2023, have a look, this should now give you a clean picture of your PR content, right? |
Hi @AdrianSosic. Yes, this view looks right to me. |
Scienfitz
left a comment
There was a problem hiding this comment.
my biggest question is why the concepts of TL and MF are now deedply intertwined by using a single searchspace property for combining them isntead of two properties characterizing the searchspaces task / fidelity character
AVHopp
left a comment
There was a problem hiding this comment.
Some more thoughts from my end
|
@jpenn2023 it seems like the continuous search space example is currently failing which is why the doc building does not run (just FYI as reading the doc building error logs can be a bit annoying) |
There was a problem hiding this comment.
I've found several comments that were marked as resolved without them actually being addressed. Please ONLY resolve comments when the changes are actually visible and have been pushed. Also, it would be great if you could give context when resolving a comment. For example, I asked an explicit question at one point and the comment was just being resolved without providing an answer and without changing the corresponding code.
EDIT: Whoopsie, this was on me. Ignore :)
AdrianSosic
left a comment
There was a problem hiding this comment.
Hey @AVHopp , here some first input. I'll now look at the kernel constructions in particular, as well as the tests
| return len(task_param.values) | ||
|
|
||
| @property | ||
| def n_fidelities(self) -> int: |
There was a problem hiding this comment.
I know you're just following the pattern of the task parameter, but I think we need to make a decision now. Much of the task stuff grew over time, but here we're now deliberately adding new functionality without having really decided how we handle the "multiple task/fidelity" case. Half of the properties/methods are private because they explicitly say private since not yet decided how to handle multiple params but then the other half is public, effectively ignoring the problem. So one of two things will happen:
- We decide that support for multi params is coming soon --> much deprecation work, i.e. all should be private
- We decide that support is far down the road --> why making it private then
There was a problem hiding this comment.
Let's discuss this point in a meeting.
There was a problem hiding this comment.
Agreed to have everything related to the search space and its properties introduced here private.
There was a problem hiding this comment.
Changed this in 3d4bd5d. Please comment if something else/additional should be private, resolve if happy.
There was a problem hiding this comment.
the analogous task properties also need to be privatized, the code is otherwise in an inconsistent state
I think this can be done without deprecation as super unimportant and super unimpactful "breaking" change, but if you disagree then do it with a deprecation
| NotImplementedError: If more than one | ||
| :class:`baybe.parameters.categorical.TaskParameter` is requested. | ||
| NotImplementedError: If more than one fidelity parameter is present. | ||
| NotImplementedError: If task and fidelity parameters are combined. |
There was a problem hiding this comment.
I think "combined" is not a good word because it sounds like you are producing one new parameter type from the two others. Same in error message
There was a problem hiding this comment.
Changed in 840dfff, please resolve if this wording is better or provide an alternative.
| if not any( | ||
| i not in (searchspace.task_idx, searchspace.fidelity_idx) | ||
| for i in range(len(searchspace.comp_rep_columns)) | ||
| ): | ||
| raise IncompatibleSurrogateError( | ||
| f"'{self.__class__.__name__}' requires at least one " | ||
| f"non-task/non-fidelity parameter." | ||
| ) |
There was a problem hiding this comment.
This is very cumbersome. Also, I'd appreciate if you could avoid access to low-level attributes like exp_rep and comp_rep since otherwise I'll have to recode this after the candidates PR 😬
In fact, no need to access such low level stuff. Simply check e.g. that there is at least one REGULAR parameter in the search space. And then also turn the message around, i.e. instead of listing non-regular types (which will break once the list get's longer), just mention that we need at least one regular parameter.
| ): | ||
| raise IncompatibleSurrogateError( | ||
| f"'{self.__class__.__name__}' does not support custom components " | ||
| f"for numerical multi-fidelity search spaces, which are delegated " |
There was a problem hiding this comment.
I think the "which" makes no sense. What should it refer to? The costum components? No --> they are not compatible. The search space? No --> botorch doesn't use our search spaces.
There was a problem hiding this comment.
Changed the formulation in d28b7e5. Please resolve if you are satisfied with the text now.
AdrianSosic
left a comment
There was a problem hiding this comment.
I'll review the kernel tests once we've aligned on the decorator stuff
| searchspace = SearchSpace.from_product(parameters) | ||
| measurements = create_fake_input( |
There was a problem hiding this comment.
why create these objrects inside the test if we have them already defined outside (keep test logic minimal)
There was a problem hiding this comment.
Can you elaborate on this? The corresponding search space objects which do not use any REGULAR parameters are not yet defined outside of the function, but the parameters are (with the excepction of the task parameter). Does it really make sense to have those search spaces and measurements all defined outside of the test on the module level or am I misunderstanding you?
Co-authored-by: AdrianSosic <adrian.sosic@merckgroup.com>
Co-authored-by: AdrianSosic <adrian.sosic@merckgroup.com>
Co-authored-by: AdrianSosic <adrian.sosic@merckgroup.com>
Co-authored-by: AdrianSosic <adrian.sosic@merckgroup.com>
65e2110 to
567a113
Compare
The decorator now represents modeling mechanisms (transfer learning, multi-fidelity) rather than the ICM kernel artifact it builds, and each mechanism can be enabled independently.
|
|
||
| ### Deprecations | ||
| - The `task_kernel_or_factory` argument of `ICMKernelFactory` has been renamed to | ||
| `index_kernel_or_factory` since it now also carries fidelity indices. The old name is |
There was a problem hiding this comment.
The last sentence The old... is obsolete as this is always implied when something is listed under Deprecations
|
|
||
|
|
||
| def _enable_transfer_learning( | ||
| def _enable_index_kernel( |
There was a problem hiding this comment.
I'm fine with this as it closes my original question, leaving it for you two to close this thread
| """The factory for the base kernel operating on numerical input features.""" | ||
|
|
||
| task_kernel_factory: KernelFactoryProtocol = field( | ||
| task_kernel_factory: KernelFactoryProtocol | None = field( |
There was a problem hiding this comment.
I'm not a huge fan of the new name as it expresses the architecture and not what is its used for (which is a more reasonable naming approach if there are choices involved)
eg we could have kernels intended for task/fidelity that are not index. but give that this is currently not the case and I have no better name suggestion feel free to resolve
| return len(task_param.values) | ||
|
|
||
| @property | ||
| def n_fidelities(self) -> int: |
There was a problem hiding this comment.
the analogous task properties also need to be privatized, the code is otherwise in an inconsistent state
I think this can be done without deprecation as super unimportant and super unimpactful "breaking" change, but if you disagree then do it with a deprecation
|
|
||
| return result | ||
|
|
||
| def _validate_fit_context( |
There was a problem hiding this comment.
personally I thought this name made sense
validate_X_context = gets everyhtign as input that X sees (or X itself as input)
Or is Adrian suggesting to make this method use _ModelContext (and rename the method accordingly)? Could also be an option if its 100% equivalent
Adding multi-fidelity properties for the SearchSpace class and multi-fidelity Gaussian process classes with the required fidelity kernels.