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 BruteForceSampler in parallel optimization #5022

Merged
merged 4 commits into from
Oct 13, 2023

Conversation

contramundum53
Copy link
Member

Motivation

Fix #4948.

Description of the changes

#4948 (comment) explains why this bug happens. This PR takes the current trial into account when inferring whether each trial is "finalized".

@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
@c-bata c-bata self-assigned this Oct 11, 2023
@c-bata c-bata added the bug Issue/PR about behavior that is broken. Not for typos/examples/CI/test but for Optuna itself. label Oct 11, 2023
@c-bata c-bata added this to the v3.4.0 milestone Oct 11, 2023
@codecov
Copy link

codecov bot commented Oct 11, 2023

Codecov Report

Merging #5022 (9375fec) into master (a0d2a0c) will decrease coverage by 0.04%.
Report is 226 commits behind head on master.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #5022      +/-   ##
==========================================
- Coverage   89.44%   89.40%   -0.04%     
==========================================
  Files         200      203       +3     
  Lines       14747    15055     +308     
==========================================
+ Hits        13190    13460     +270     
- Misses       1557     1595      +38     
Files Coverage Δ
optuna/samplers/_brute_force.py 97.16% <100.00%> (+2.88%) ⬆️

... and 48 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

@nabenabe0928 Could you review this PR?

@nabenabe0928
Copy link
Collaborator

@nabenabe0928 Could you review this PR?

Yes, please assign me:)

Copy link
Collaborator

@nabenabe0928 nabenabe0928 left a comment

Choose a reason for hiding this comment

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

Test looks good to me and I am checking the sampler.

Copy link
Collaborator

@nabenabe0928 nabenabe0928 left a comment

Choose a reason for hiding this comment

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

I added a comment for future maintenance, but your change looks good to be otherwise:)

optuna/samplers/_brute_force.py Show resolved Hide resolved
Co-authored-by: Shuhei Watanabe <47781922+nabenabe0928@users.noreply.github.com>
optuna/samplers/_brute_force.py Outdated Show resolved Hide resolved
Co-authored-by: Shuhei Watanabe <47781922+nabenabe0928@users.noreply.github.com>
@nabenabe0928 nabenabe0928 removed their assignment Oct 12, 2023
Copy link
Member

@c-bata c-bata left a comment

Choose a reason for hiding this comment

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

LGTM.

@c-bata c-bata merged commit 0061a42 into optuna:master Oct 13, 2023
32 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Issue/PR about behavior that is broken. Not for typos/examples/CI/test but for Optuna itself. optuna.samplers Related to the `optuna.samplers` submodule. This is automatically labeled by github-actions.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

BruteForceSampler crashes when used with n_jobs
3 participants