-
-
Notifications
You must be signed in to change notification settings - Fork 971
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
Loose coupling nsgaii elite population selection #4821
Loose coupling nsgaii elite population selection #4821
Conversation
8fb39b3
to
d511a75
Compare
@HideakiImamura Could you review this PR, please? |
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 PR. I have two comments. PTAL.
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.
Just for confirmation. The loose coupling of the elite population selection for the NSGAIIISampler
will be done as a follow-up?
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.
Yes, I am going to do it as a follow-up.
Codecov Report
❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more. @@ Coverage Diff @@
## master #4821 +/- ##
==========================================
+ Coverage 89.53% 89.55% +0.01%
==========================================
Files 197 198 +1
Lines 14671 14689 +18
==========================================
+ Hits 13135 13154 +19
+ Misses 1536 1535 -1
... and 13 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
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.
Thank you for your PR. Let me share my early comments.
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 have a comment on the reference. I guess the strategies are still private.
optuna/samplers/nsgaii/_sampler.py
Outdated
.. note:: | ||
|
||
The arguments ``elite_population_selection_strategy``, | ||
``child_generation_strategy``, and ``after_trial_strategy`` was added in v3.3.0 as | ||
an experimental feature. The interface may change in newer versions without prior | ||
notice. See https://github.com/optuna/optuna/releases/tag/v3.3.0. |
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.
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.
LGTM except @toshihikoyanase's comment.
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.
Thank you for your update. The readthedocs
CI job failed, but I believe it is unrelated to the changes in this PR. LGTM!
Motivation
Progress v3.3 task to make
NSGAII Sampler
loose coupled.Description of the changes
Add
_elite_population_selection_strategy
argument to the constructor ifNSGAIISampler
and separate the elite population selection process so that users can customize surviving process.Please merge this PR after #4806 is merged.