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

Fix duplicated sampling of SkoptSampler #4023

Merged
merged 2 commits into from
Oct 4, 2022
Merged

Fix duplicated sampling of SkoptSampler #4023

merged 2 commits into from
Oct 4, 2022

Conversation

not522
Copy link
Member

@not522 not522 commented Sep 28, 2022

Motivation

Fix #4022.
SkoptSampler samples duplicated parameters because _Optimizer is created using the same random seed.

Description of the changes

Update random_state after creating _Optimizer.

@github-actions github-actions bot added the optuna.integration Related to the `optuna.integration` submodule. This is automatically labeled by github-actions. label Sep 28, 2022
@not522 not522 added the bug Issue/PR about behavior that is broken. Not for typos/examples/CI/test but for Optuna itself. label Sep 28, 2022
@not522 not522 mentioned this pull request Sep 28, 2022
Closed
@codecov-commenter
Copy link

Codecov Report

Merging #4023 (c6af83b) into master (58434f9) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #4023   +/-   ##
=======================================
  Coverage   90.19%   90.20%           
=======================================
  Files         160      160           
  Lines       12580    12582    +2     
=======================================
+ Hits        11347    11349    +2     
  Misses       1233     1233
Impacted Files Coverage Δ
optuna/integration/skopt.py 98.15% <100.00%> (+0.02%) ⬆️

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

rene-rex
rene-rex approved these changes Sep 28, 2022
View reviewed changes
Copy link

@rene-rex rene-rex left a comment

Choose a reason for hiding this comment

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

Works for me.

@toshihikoyanase
Copy link
Member

@contramundum53 Could you review this PR, please?

contramundum53
contramundum53 approved these changes Oct 3, 2022
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!

@contramundum53 contramundum53 removed their assignment Oct 3, 2022
toshihikoyanase
toshihikoyanase approved these changes Oct 4, 2022
View reviewed changes
Copy link
Member

@toshihikoyanase toshihikoyanase left a comment

Choose a reason for hiding this comment

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

LGTM.

@toshihikoyanase toshihikoyanase merged commit 7da2f2d into optuna:master Oct 4, 2022
@toshihikoyanase toshihikoyanase added this to the v3.1.0 milestone Oct 4, 2022
@not522 not522 deleted the fix-skopt-seed branch October 20, 2022 03:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@contramundum53 contramundum53 contramundum53 approved these changes

@rene-rex rene-rex rene-rex approved these changes

@toshihikoyanase toshihikoyanase toshihikoyanase approved these changes

Assignees

@toshihikoyanase toshihikoyanase

Labels
bug Issue/PR about behavior that is broken. Not for typos/examples/CI/test but for Optuna itself. optuna.integration Related to the `optuna.integration` submodule. This is automatically labeled by github-actions.
Projects
None yet
Milestone
v3.1.0-b0
Development

Successfully merging this pull request may close these issues.

If seed is fixed, SkoptSampler suggests exactly the same parameter sets during startup trials
5 participants
@not522 @codecov-commenter @toshihikoyanase @rene-rex @contramundum53