-
Notifications
You must be signed in to change notification settings - Fork 69
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
Support CMA-ES with Margin. #121
Support CMA-ES with Margin. #121
Conversation
3c9dc05
to
9bdfb09
Compare
Thank you for your pull request! @nomuramasahir0 Could you trigger the GitHub action on this PR? |
I opened #122 to fix CI problems 🙇 |
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.
Changes overall looks good to me and the current interface looks more straightforward than #115.
We might want to consider refactoring duplicated code with CMA
class, but as for this change, the current implementation looks good to me. We can work on it as follow-up tasks.
I left two minor comments.
from scipy.stats import chi2 | ||
from scipy.stats import norm |
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.
Should we add scipy
in extra_requires
?
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 added cmawm = scipy
. ( b3a9608)
Thanks for the review! I applied your comments. I'm sorry for the PR with a large change. Please see Although the behavior with the same seed is different from #115 (because I changed the order of internal representations), I checked that we can obtain similar tendencies in benchmark problems. 2000 trials, 100 runs for each sampler in each problem: |
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.
Thanks for the update! LGTM!
The implementation of CMA-ES with margin by #115 has the following issues.
__init__
is inconsistent withCMA
.This PR is a continuation of #115 and changes the interface of
CMAwM
to address the issues. The example code would be changed as follows.