-
Notifications
You must be signed in to change notification settings - Fork 28
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.
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
kernels working on a given set of features #476
Conversation
TODOs:
|
Example with mol features: import pandas as pd
import bofire.strategies.api as strategies
from bofire.data_models.domain import api as domain_api
from bofire.data_models.features import api as features_api
from bofire.data_models.kernels import api as kernels_api
from bofire.data_models.molfeatures import api as molfeatures_api
from bofire.data_models.priors.api import HVARFNER_LENGTHSCALE_PRIOR
from bofire.data_models.strategies import api as strategies_api
from bofire.data_models.surrogates import api as surrogates_api
domain = domain_api.Domain(
inputs=domain_api.Inputs(
features=[
features_api.ContinuousInput(key="x1", bounds=(-1, 1)),
features_api.ContinuousInput(key="x2", bounds=(-1, 1)),
features_api.CategoricalMolecularInput(
key="mol", categories=["CO", "CCO", "CCCO"]
),
]
),
outputs=domain_api.Outputs(features=[features_api.ContinuousOutput(key="f")]),
)
strategy = strategies.map(
strategies_api.SoboStrategy(
domain=domain,
surrogate_specs=surrogates_api.BotorchSurrogates(
surrogates=[
surrogates_api.SingleTaskGPSurrogate(
inputs=domain.inputs,
outputs=domain.outputs,
input_preprocessing_specs={
"mol": molfeatures_api.Fingerprints(),
},
kernel=kernels_api.AdditiveKernel(
kernels=[
kernels_api.RBFKernel(
ard=True,
lengthscale_prior=HVARFNER_LENGTHSCALE_PRIOR(),
features=["x1", "x2"],
),
kernels_api.TanimotoKernel(
features=["mol"],
),
]
),
)
]
),
)
)
strategy.tell(
experiments=pd.DataFrame(
[
{"x1": 0.2, "x2": 0.4, "mol": "CO", "f": 1.0},
{"x1": 0.4, "x2": 0.2, "mol": "CCO", "f": 2.0},
{"x1": 0.6, "x2": 0.6, "mol": "CCCO", "f": 3.0},
]
)
)
candidates = strategy.ask(candidate_count=1)
print(candidates)
# x1 x2 mol f_pred f_sd f_des
# 0 1.0 1.0 CCCO 2.932646 0.945029 2.932646 |
@e-dorigatti: This looks really great and will make the GP section so much easier, we can then get rid of a lot of GP classes! Thank you very much! |
I think, a specific challenge but also a good opportunity to test will be the The question is how we deal with this? One option would be to code a new Hamming distance kernel which also works on one hot encodeded categoricals or to also start to model the input transforms on a feature specific level in the datamodel. This would then also solve the PiecewiselinearGP but would add more complexity but also versatility. The mapping of the features for the input transforms could be done in the same way as you are currently doing it for the kernels. What do you think? I have the feeling that exposing also the input transforms would be the cleanest solution. What is difficult here, is that the |
Hi @jduerholt thanks for the kind feedback and pointers :) this sounds like a very important use-case to implement and in the next weeks I will look into it. My first idea would be to go via the Going through an input transform sounds (a lot?) more involved, why do you think that would be preferable to the preprocessing specs? Maybe some kernel/features combinations are handled more efficiently in botorch? For example, it would make a lot of sense if a Hamming kernel did not need to actually expand the one-hot encoding. But this I could also implement myself. Maybe I need to learn more about botorch first :) |
Hi @e-dorigatti, a few more notes/explanations from my side:
In general, botorch input transforms are a great way to build up complex differentiable optimization pipelines, on the other hand it is very challenging to come up with a general framework, parameterizied by feature names that can compose different input transforms on different features including the bookkeeping of indices. I hope his posts helps ;) |
Thank you for the thorough explanation @jduerholt, this clears up a lot of doubts! So the key difference is that the optimizer of the acquisition function is working with the features that result after applying the input preprocessing specs, but before the input transform. What are the reasons that led you to decide that categoricals have to be one-hotted? Currently because I don't know your motivations it looks a bit silly since you use the input transform to do the one-hot, then the OneHotToNumeric to undo the one-hot, as the Hamming kernel does not need one-hots in the first place. If we now add more code to deal with the fact that this specific transform (and only this one?) changes the input dimensionality, it would seem like over-complicating things even further (but it would not be that complicated in the end). Maybe, given that with this PR you can have kernels focusing on specific (subsets of) features, it is now unnecessary to force categoricals to be one-hot? Moreover, because molecules are in the end a special type of categorical, it would make sense imho to convert the molecular featurizers to transforms rather than preprocessing. I am now wondering whether it is necessary to expose botorch's input transforms, or it would be okay to put these transformations within the kernels themselves, so that users would specify the features that the kernel is acting on as well as how they should be transformed. I think this would be simpler to implement, and not less powerful than botorch's input transforms unless I am missing something. It also makes sense because, in some way, kernels are the only reason why we need input transforms in the first place, and different kernels could work on different representations of the same feature. What do you think? In the end, I think that both options can be implemented relatively easily, but perhaps one is more disruptive than the other. I think you have a better perspective and ability to judge :) |
Will provide you a thorough answer tomorrow ;) |
The reason was that I wanted to have things too flexible:
You are right, they are special forms of the categorical features. But I would not convert them to botorch input transforms, for the following reasons:
Two reasons for keeping it:
What I would do now:
This should overall, simplify our codebase a lot and will give in general much more flexibility. Of course, there will be also edge cases. What do you think? Ofc, this should not all be done in this PR by you. So I would suggest, that we try to get this PR (which does not breaks anything) in as fast as possible, agree on the changes from above and then distribute the taks. Fine for you? |
Thank you for the clarifications @jduerholt. Following your suggestions I have implemented the Hamming kernel working on one-hot features, and verified that one can get the same results as MixedSingleTaskGP (or even better with a different kernel). The kernel mapper decides transparently from the user whether to use this kernel or the one from botorch depending on whether feature names are specified (SingleTaskGP), or not (MixedSingleTaskGP). |
…n-feature-subsets
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @e-dorigatti,
thank you very much for the additions and sorry for the delay in review.
I have some doubts that the new kernel does exactly the same as the old one. See my comments. If I am correct, I would propose the following next steps, as I am also assuming that it makes too much efforts to come up with a correct version:
- Remove the new kernel
- Merge it as is with the new feature dependend setup as new expert feature
- I will implement then the suggestions that I made last year to lift the restriction that categorical features has to always be one-hot encoded.
What do you think?
Best,
Johannes
Hi @jduerholt, sorry for the mess, I was probably too eager to start vacation :) I revised the implementation of the kernel to use the one-hot to numeric transformation and added tests for using multiple categoricals at the same time both with and without ARD I also took care of your other comments about renaming, docstrings, redundant checks, etc. Let me know if you spot any other issues. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@e-dorigatti: Looks good to me. Only some minor stuff. Thanks!
After this PR is merged, we can simplify a lot in BoFire ;)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perfect @e-dorigatti! Thank you very much. Based on this, we should be able to tidy up a lot in BoFire!
Fantastic, thank you @jduerholt for the support and guidance, happy to have contributed :) |
* kernels working on a given set of features * pre-commit * test map singletaskgp with additive kernel * test active_dims of mapped kernels * add features_to_idx_mapper to outlier detection tutorial * correctly handling categorical mol features * validating mol features transforms * verifying proper type * custom hamming kernel enabling single task gp on categorical features * removed unnecessary parameter from data model * testing equivalence of mixed gp and single gp with custom kernel * (temporary) running on all py versions * (temporary) debug github actions by printing * more printing * Revert "testing equivalence of mixed gp and single gp with custom kernel" This reverts commit 4a2a547. * Revert "removed unnecessary parameter from data model" This reverts commit 6ad1dfd. * Revert "custom hamming kernel enabling single task gp on categorical features" This reverts commit 17d8350. * Revert "Revert "custom hamming kernel enabling single task gp on categorical features"" This reverts commit 2e29852. * Revert "Revert "testing equivalence of mixed gp and single gp with custom kernel"" This reverts commit 1cd2776. * removed test debug and restored to latest implemented features * pinning compatible version of formulaic * pinning compatible version of formulaic * removed old code * lint * removed scratch file * removed old code again * silencing pyright false positive * compatibility with py39 * pin compatible version of formulaic * restored old code * pinning sklearn * pinning sklearn * pinning scikit everywhere * not testing for prediction quality * matching lengthscale constraints in hamming kernel * removed equivalence test * testing hamming kernel * added test for mol features in single task gp * categorical onehot kernel uses the right lengthscale for multiple features * removed redundant check * more descriptive name for base kernel * updated docstring * improved tests and comments --------- Co-authored-by: Robert Lee <[email protected]>
* add draft of restrucuted doe class * refactoring doe * add formulaic to be installed always * add formulaic to be installed always * add formulaic to be installed always * add formulaic to be installed always * check style * check style * check style * remove enumns * remove enumns * remove enumns * fix branch and bound * move delta into criterion * move delta into criterion * move delta into criterion * move delta into criterion * move default criterion * move default criterion * move default criterion * move default criterion * refactor formulas and number of experiments * pyright * fix test * fix test * fix test * fix tutorial * fix tutorial * fix tutorial * fix test * fix test * fix getting started * aarons review * rmv unneded tests * formulaic version fixed bc of breaking changes * add explanatory text to doe basic examples * typo in basic_examples.ipynb * format basic doe example * consolidate spac_filling with doe * Add categoricals for `FractionalFactorialStrategy` (#480) * integrate factorial in fractional factorial * fix tests * merge main * Multiplicative additive sobo objectives (#481) * added MultiplicativeAdditive data model * added actual multiplicative model (callable is missing) * added torch functions * added test for objective * added test for sobo stategy: multiplicative_additive * changed additive/multiplicative calculations: - Removed scaling by x**(1/(....)) to avoid numerical errors, if x<0 - included weight transformation for multiplicative objectives from (0, 1] to [1, inf) scale to avoid numerical errors at weights != 1.0 - added tests for weights != 1.0 * added notebook for comparison of merging objectives * after hooks * addet .idea/ folder (pycharm) to gitignore * after hooks * Apply pre-commit fixes * Delete .idea directory * corrected tests for multiplicative_additive_botorch_objective * after pre-commit * lint specifications * corrected weightings calc in test for multiplicative objective * after hooks * changed docstrings to google docstring format * easy fixes, spelling errors * forgot linting * easy fixes, spelling errors * removed denominator additive from multiplicative_additive_sobo strategy * after hooks * fixed typing * tensor initialization of objectives * after hooks * avoiding torch size error * avoid linting error * after hooks * reverting test-renaming * revert isinstance list comprehension to tuple.... solution * testing copilot suggestions for linting errors * reverting wrong copilot suggestions * added test for _callables_and_weights * after hooks * added test for SOBO strategy data model * added test for SOBO strategy data model * added new sobo strategy to a mysterious list * after hooks * still trying to get rid of the linting error, expecting tuple(types) * WIP * WIP * WIP * WIP * WIP * minor corrections * add pbar support for hyperopt (#494) * Make the BoFire Data Models OpenAI compatible (#495) * tuples to lists * fix tests * fix linting issues * Group split kfold (#484) * add group kfold option in cross_validate of any traainable surrogate * changed to GroupShuffleSplit, added test case * improve docstring & add some inline comments in test * refactor cross_validate & add tests * imrpve tests, remove unnecessary case while checking group split col * add push * formatting --------- Co-authored-by: Jim Boelrijk Valcon <[email protected]> * fix strict candidate enforcement (#492) * Drop support for Python 3.9 (#493) * update tests and pyproject.toml * update lint workflow * update test * bump pyright * different pyright version * change linting * Update pyproject.toml (#501) BoTorch is slowed down massively by scipy 1.15: pytorch/botorch#2668. We should fix it. * kernels working on a given set of features (#476) * kernels working on a given set of features * pre-commit * test map singletaskgp with additive kernel * test active_dims of mapped kernels * add features_to_idx_mapper to outlier detection tutorial * correctly handling categorical mol features * validating mol features transforms * verifying proper type * custom hamming kernel enabling single task gp on categorical features * removed unnecessary parameter from data model * testing equivalence of mixed gp and single gp with custom kernel * (temporary) running on all py versions * (temporary) debug github actions by printing * more printing * Revert "testing equivalence of mixed gp and single gp with custom kernel" This reverts commit 4a2a547. * Revert "removed unnecessary parameter from data model" This reverts commit 6ad1dfd. * Revert "custom hamming kernel enabling single task gp on categorical features" This reverts commit 17d8350. * Revert "Revert "custom hamming kernel enabling single task gp on categorical features"" This reverts commit 2e29852. * Revert "Revert "testing equivalence of mixed gp and single gp with custom kernel"" This reverts commit 1cd2776. * removed test debug and restored to latest implemented features * pinning compatible version of formulaic * pinning compatible version of formulaic * removed old code * lint * removed scratch file * removed old code again * silencing pyright false positive * compatibility with py39 * pin compatible version of formulaic * restored old code * pinning sklearn * pinning sklearn * pinning scikit everywhere * not testing for prediction quality * matching lengthscale constraints in hamming kernel * removed equivalence test * testing hamming kernel * added test for mol features in single task gp * categorical onehot kernel uses the right lengthscale for multiple features * removed redundant check * more descriptive name for base kernel * updated docstring * improved tests and comments --------- Co-authored-by: Robert Lee <[email protected]> * Fix mapper tests (#502) * fix kernel mapper tests * bump botorch dependency * rmv unused import in staregies.api * rmv unused import in space filling * rmv unused import in space filling * fix data models tets * fix data models tets * fix data models tets * fix data models tets * fix data models tets * no more bnb in test * add fixtures for criteria * add fixtures for criteria --------- Co-authored-by: LinzneDD_basf <[email protected]> Co-authored-by: Dominik Linzner <[email protected]> Co-authored-by: linznedd <[email protected]> Co-authored-by: Robert Lee <[email protected]> Co-authored-by: Lukas Hebing <[email protected]> Co-authored-by: Julian Keupp <[email protected]> Co-authored-by: Jim Boelrijk Valcon <[email protected]> Co-authored-by: Emilio Dorigatti <[email protected]>
No description provided.