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.

Sign up for GitHub

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

Jump to bottom

Support constant_liar in multi-objective TPESampler #5021

Merged
merged 6 commits into from Oct 27, 2023
Merged

Support constant_liar in multi-objective TPESampler #5021

merged 6 commits into from Oct 27, 2023

Conversation

nabenabe0928
Copy link
Collaborator

Motivation

Support constant_liar for multi-objective TPE as mentioned in #3565 .

Description of the changes

As running_trials is simply added to samples_above in the current implementation, we just need to remove the branching in the if-statement for multi-objective and constant_liar.

@github-actions github-actions bot added the optuna.samplers Related to the `optuna.samplers` submodule. This is automatically labeled by github-actions. label Oct 11, 2023
@codecov
Copy link

codecov bot commented Oct 11, 2023 •
edited

Codecov Report

Merging #5021 (583c8ec) into master (3fcbf88) will decrease coverage by 0.01%.
Report is 58 commits behind head on master.
The diff coverage is 100.00%.

❗ Current head 583c8ec differs from pull request most recent head 24245ef. Consider uploading reports for the commit 24245ef to get more accurate results

@@            Coverage Diff             @@
##           master    #5021      +/-   ##
==========================================
- Coverage   89.40%   89.39%   -0.01%     
==========================================
  Files         203      203              
  Lines       15055    15055              
==========================================
- Hits        13460    13459       -1     
- Misses       1595     1596       +1
Files Coverage Δ
optuna/samplers/_tpe/sampler.py 98.07% <100.00%> (ø)

... and 2 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@c-bata
Copy link
Member

c-bata commented Oct 12, 2023

@knshnb @contramundum53 Could you review this PR?

@nabenabe0928
Copy link
Collaborator Author

I will add a test later.

@nabenabe0928 nabenabe0928 force-pushed the 3565/feature/support-constant-liar-in-motpe branch from d7772ca to 52000cf Compare October 13, 2023 01:30
contramundum53
contramundum53 approved these changes Oct 13, 2023
View reviewed changes
Copy link
Member

@contramundum53 contramundum53 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@nabenabe0928 nabenabe0928 force-pushed the 3565/feature/support-constant-liar-in-motpe branch from 91e5d04 to fbf342c Compare October 16, 2023 03:46
@not522
Copy link
Member

not522 commented Oct 19, 2023

Could you remove this note?
image

@nabenabe0928
Copy link
Collaborator Author

Could you remove this note? image

Removed!

@github-actions
Copy link
Contributor

This pull request has not seen any recent activity.

@github-actions github-actions bot added the stale Exempt from stale bot labeling. label Oct 26, 2023
knshnb
knshnb reviewed Oct 27, 2023
View reviewed changes
Copy link
Member

@knshnb knshnb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the delayed response... Almost LGTM except for one comment.

optuna/samplers/_tpe/sampler.py Outdated
@@ -646,6 +642,7 @@ def _split_complete_trials_multi_objective(
n_below: int,
) -> tuple[list[FrozenTrial], list[FrozenTrial]]:
if n_below == 0:
# QUESTION: Shall I change it as well??
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This does not seem to be related to this PR, so shall we handle this as another issue?

knshnb
knshnb approved these changes Oct 27, 2023
View reviewed changes
Copy link
Member

@knshnb knshnb left a 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!

@knshnb knshnb enabled auto-merge October 27, 2023 08:19
@knshnb knshnb merged commit da9ec3d into optuna:master Oct 27, 2023
30 checks passed
@not522 not522 mentioned this pull request Oct 27, 2023
Closed
3 tasks
@nabenabe0928 nabenabe0928 deleted the 3565/feature/support-constant-liar-in-motpe branch November 8, 2023 07:00
@nabenabe0928 nabenabe0928 added this to the v3.5.0 milestone Dec 8, 2023
@HideakiImamura HideakiImamura added enhancement Change that does not break compatibility and not affect public interfaces, but improves performance. feature Change that does not break compatibility, but affects the public interfaces. labels Dec 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@knshnb knshnb knshnb approved these changes

@contramundum53 contramundum53 contramundum53 approved these changes

Assignees

@knshnb knshnb

@contramundum53 contramundum53

Labels
enhancement Change that does not break compatibility and not affect public interfaces, but improves performance. feature Change that does not break compatibility, but affects the public interfaces. optuna.samplers Related to the `optuna.samplers` submodule. This is automatically labeled by github-actions. stale Exempt from stale bot labeling.
Projects
None yet
Milestone
v3.5.0
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@nabenabe0928 @c-bata @not522 @knshnb @contramundum53 @HideakiImamura