Skip to content
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

fix(GPHedge): replace list with sequence #524

Conversation

phi-friday
Copy link
Contributor

Given the use case below, it seems that base_acquisitions in GPHedge can have multiple types of AcquisitionFunction.

acq = acquisition.GPHedge(base_acquisitions=base_acquisitions)

However, the type args in list are fixed, so they do not allow such a situation.
Therefore, we need to change to Sequence, which allows args with covariant=True.
I was going to ignore it because it's related to generics,
but it seemed necessary for the use case you're considering, so I fixed it.

Note

This is not a complete description.
But I think it will help you understand why the error is occurring.

  • list[T: AcquisitionFunction]: All elements in list are T.

correct case: [UpperConfidenceBound] does belong to list[UpperConfidenceBound].(Also, UpperConfidenceBound is a subclass of AcquisitionFunction.)
error case: [UpperConfidenceBound, ProbabilityOfImprovement] does not belong to any of the three sets list[AcquisitionFunction], list[UpperConfidenceBound], or list[ProbabilityOfImprovement].

  • Sequence[T_co: AcquisitionFunction]: Every element in Sequence is either T_co or a subclass of T_co.

[UpperConfidenceBound, ProbabilityOfImprovement] does not belong to Sequence[UpperConfidenceBound] or Sequence[ProbabilityOfImprovement], but it does belong to Sequence[AcquisitionFunction].

@till-m till-m merged commit f63372e into bayesian-optimization:master Sep 30, 2024
11 checks passed
@till-m
Copy link
Member

till-m commented Sep 30, 2024

Thanks for your contribution!

@phi-friday phi-friday deleted the fix-gphedge-replace-list-with-sequence branch September 30, 2024 11:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants