-
Notifications
You must be signed in to change notification settings - Fork 48
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
Pareto optimization #475
base: main
Are you sure you want to change the base?
Pareto optimization #475
Conversation
385bebe
to
e633f92
Compare
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.
first round of comments
27ae08c
to
711935f
Compare
44956f9
to
01dbb6f
Compare
Co-authored-by: Martin Fitzner <[email protected]>
The ref_point is now in the original target space so that the user can intuitively specify its coordinates. Sign flips for minimization targets happen behind the scenes.
01dbb6f
to
b3b77fb
Compare
): | ||
raise IncompatibleAcquisitionFunctionError( | ||
f"You attempted to use a single-target acquisition function in a " | ||
f"{n_targets}-target context." |
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.
the language here is not good due to the same issue elsewhere: a single target acqf is perfectly fine eg for a desirability objective (ie multiple targets), whats really the problem is the number of outputs (and not targets)
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.
detached comment 3: working with Pareto I think the Campaign.posterior
call is much more important and probaly part fo the workflow (to check the target predictions and possibly make a subchoice of poitns to evaluate on the predicted frontier). Imo this then should be mentioned both in the UG and in the example (albeit briefy, jsut referencing the posterior
method and explaiing why its useful for that)
# Validate multi-target compatibility | ||
if not self.supports_multi_target and (n_targets := len(objective.targets)) > 1: | ||
raise IncompatibleSurrogateError( | ||
f"You attempted to train a single-target surrogate in a " |
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.
f"You attempted to train a single-target surrogate in a " | |
f"You attempted to train a single-output surrogate in a " |
|
||
|
||
@define | ||
class BroadcastingSurrogate(SurrogateProtocol): |
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.
I was expecting a supports_multi_target=True in this class
but perhaps its not needed or you had other reasons?
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.
Yesterday I answered "yes, thanks, forgot" but I just realize that I omitted it on purpose since actually not needed / fulfills no purpose here. The potential confusion is: note that the two composite classes are not subclasses of Surrogate
(which is where the flag is declared), they only implement the Protocol
. Because the flag is only used inside Surrogate
, there is no benefit of also exposing a copy of it in the composite ones – they will at some point dispatch the the actual surrogates, which is where the check is then performed (and if coming via this route, it will not even be required since the number of encountered targets is then trivially 1). So it's only ever needed if you don't go via the composite surrogates. Makes sense? If yes, please resolve
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.
I just thought having such flag available outside of Surrogate
could be useful, think eg about tests
The reference point is positioned in relation to the worst target configuration | ||
within the provided array. The distance in each target dimension is adjusted by | ||
a specified multiplication factor, which scales the reference point away from | ||
the worst target configuration based on the maximum observed differences in | ||
target values. |
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.
The reference point is positioned in relation to the worst target configuration | |
within the provided array. The distance in each target dimension is adjusted by | |
a specified multiplication factor, which scales the reference point away from | |
the worst target configuration based on the maximum observed differences in | |
target values. | |
The reference point is positioned relative to the worst point in the direction coming from the best | |
point. A factor of 0.0 would result in the reference point being the worst point, while a factor > 0.0 | |
would move the reference point further away from both worst and best points. A factor of 1.0 would | |
exactly mirror the best on on the worst point. |
plt.ylabel(y1.name) | ||
plt.title("Target Space") | ||
plt.axis("square") | ||
|
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.
can we mix target types in this example? ie one max and one min
reason: its safer to test mixed targets as both cases for the multiplier are contained
|
||
```{admonition} Non-dominated Configurations | ||
:class: tip | ||
A target configuration is considered non-dominated if no other configuration is better |
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.
for a newbie otherwise its probably not clear why the objective is even called Pareto
A target configuration is considered non-dominated if no other configuration is better | |
A target configuration is considered non-dominated (or Pareto-optimal) if no other configuration is better |
|
||
|
||
@define | ||
class CompositeSurrogate(SurrogateProtocol): |
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.
is there any merit to deriving BroadcastingSurrogate
from CompositeSurrogate
?
It seems like a special case to me
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.
No, inheritance does not help here – the attribute layout and way of calling the initializer is different. But I know what you had in mind: a BroadcastingSurrogate
indeed behaves like a CompositeSurrogate
with a flexible collection of single-task surrogates. So what you probably meant – and what indeed work – is to compose the CompositeSurrogate
in that case via an alternative construction route:
@classmethod
def from_template(cls, surrogate: SurrogateProtocol) -> CompositeSurrogate:
return cls(defaultdict(lambda: deepcopy(surrogate)))
With that, doing a BroadcastingSurrogate(template)
would be identical to CompositeSurrogate.from_template(template)
.
In fact:
- the amount of code required would be significantly less (i.e. one entire class dropped)
- everything should still work via the API since we have our clever
constructor
flag - we could still use the auto-conversion for single-target surrogates
But the consequence is that the creation step would no longer be a regular "class call" but requires this separate route instead. So which do we prefer?
if not self.supports_multi_output and (n_targets := len(objective.targets)) > 1: | ||
raise IncompatibleSurrogateError( | ||
f"You attempted to train a single-target surrogate in a " | ||
f"{n_targets}-target context. Either use a proper multi-target " |
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.
f"{n_targets}-target context. Either use a proper multi-target " | |
f"context requiring {n_targets} model outputs. Either use a proper multi-output " |
|
||
|
||
@define | ||
class BroadcastingSurrogate(SurrogateProtocol): |
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.
not mentioned anywhere in the surrogate userguide so far, but I think these are important explanations.
Im not calling for a complete revamp, but at least mention that the models currently mentioned are all single-output (with the exception of GP where it uses the builtin botorch mechanism)
and then mention the current ways of creating multi-output models via composite or broadcastin from the above
long term (not for this PR) I can imagine a section explaining (perhaps even with some flow charts) the four multi-output possibilities
- botorch builtin broadcasting (comp efficient, no partial measurements)
- broadcasting a single-target model, supports partial measurements
- composite, supports partial targets
- an inherently multi-output model not consisting of any independent models
do I understand it right that after this PR we already have possibilities 1 to 3 (without the partial targets of course)
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.
besides acqfs ad surrogates, what about also giving all objectives a property supports_multi_output
?
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.
if we have a reason for it –> why not. but if not, why add unnecessary code to maintain? what use case do you have in mind?
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.
use case in mind is mostly tests and any kind of sutiation where i need to distinguish that property programatically
i also think the objectives are prob one of the easiest to maintain this flag
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.
I could imagine for instance doc pages that automatically iterate over avaialble objects,g rouping them into multi-output / single-output etc
the other usecase is always tests
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.
OK 👍🏼 but in that case I suggest we do the usual thing: we add it in that second when it's needed, instead of doing the potentially this might be helpful
-logic
This PR finally brings Pareto optimization via the new
ParetoObjective
class, together with an example comparing Pareto vs. single target optimization for a simple pair of synthetic targets.Note: Support is currently limited to maximization and minimization targets. Match targets will follow but require a refactoring of the corresponding target transformation mechanism.