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

Add tests for CmaEsSampler #4233

Merged
merged 3 commits into from
Dec 21, 2022
Merged

Add tests for CmaEsSampler #4233

merged 3 commits into from
Dec 21, 2022

Conversation

c-bata
Copy link
Member

@c-bata c-bata commented Dec 9, 2022 •
edited
Loading

Motivation

Follow-up #4184.

Let me this PR ready for review after merged #4184.

Description of the changes

Add test cases to verify that CmaEsSampler restores the correct optimizer and updates the distribution with the correct solution trials.

@c-bata c-bata marked this pull request as draft December 9, 2022 12:39
@github-actions github-actions bot added the optuna.samplers Related to the `optuna.samplers` submodule. This is automatically labeled by github-actions. label Dec 9, 2022
@codecov-commenter
Copy link

codecov-commenter commented Dec 12, 2022 •
edited
Loading

Codecov Report

Merging #4233 (944a24b) into master (bd4eaf9) will increase coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #4233      +/-   ##
==========================================
+ Coverage   89.94%   89.95%   +0.01%     
==========================================
  Files         169      170       +1     
  Lines       13274    13279       +5     
==========================================
+ Hits        11939    11945       +6     
+ Misses       1335     1334       -1
Impacted Files Coverage Δ
optuna/samplers/_cmaes.py 99.12% <100.00%> (+0.44%) ⬆️
optuna/integration/botorch.py 96.52% <0.00%> (-0.87%) ⬇️
optuna/samplers/_tpe/sampler.py 96.68% <0.00%> (-0.21%) ⬇️
optuna/_hypervolume/__init__.py 100.00% <0.00%> (ø)
optuna/_hypervolume/hssp.py 100.00% <0.00%> (ø)
optuna/study/study.py 95.71% <0.00%> (+0.77%) ⬆️

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

@c-bata c-bata marked this pull request as ready for review December 12, 2022 03:28
@c-bata c-bata requested review from gen740 and removed request for gen740 December 12, 2022 04:02
@c-bata c-bata added the code-fix Change that does not change the behavior, such as code refactoring. label Dec 12, 2022
@c-bata
Copy link
Member Author

c-bata commented Dec 12, 2022

@gen740 @contramundum53 Could you review this PR if you have time?

gen740
gen740 reviewed Dec 19, 2022
View reviewed changes
tests/samplers_tests/test_cmaes.py
Comment on lines +413 to +416
def objective(trial: optuna.Trial) -> float:
x1 = trial.suggest_float("x1", -10, 10, step=1)
x2 = trial.suggest_float("x2", -10, 10)
return x1**2 + x2**2
Copy link
Collaborator

Choose a reason for hiding this comment

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

This objective function appears so many times, how about defining this function on the top of this test?

Copy link
Member Author

@c-bata c-bata Dec 19, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

@gen740 Thank you for your review!

I understand your motivation. Obviously, this is not optimal in DRY mind. It's a matter of preference, but reusing the same objective function in multiple test cases often made unit tests non-readable. (e.g. prepare_study_with_trials function, which is used in visualization tests, contains a lot of implicit specifications and lots of unit tests depends on it. See https://github.com/optuna/optuna/pulls?q=is%3Apr+author%3Ac-bata+prepare_study_with_trials for details.)

So if it's a not your strong opinion, I suggest keeping this function not DRY. What do you think?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I see. Indeed, reusing an objective function is sometimes hard to read...
I think that's fine to define for each test.

contramundum53
contramundum53 approved these changes Dec 19, 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!

gen740
gen740 approved these changes Dec 21, 2022
View reviewed changes
Copy link
Collaborator

@gen740 gen740 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
Copy link
Member Author

c-bata commented Dec 21, 2022

@contramundum53 Please merge this PR since this PR has two approvals.

@c-bata c-bata added this to the v3.1.0 milestone Dec 21, 2022
@contramundum53 contramundum53 merged commit bbcbc07 into optuna:master Dec 21, 2022
@c-bata c-bata deleted the cmaes-sys-attr-key branch December 21, 2022 08:34
@not522 not522 changed the title Add tests for CmaEsSampler. Add tests for CmaEsSampler Jan 15, 2023
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

@gen740 gen740 gen740 approved these changes

Assignees

@contramundum53 contramundum53

@gen740 gen740

Labels
code-fix Change that does not change the behavior, such as code refactoring. optuna.samplers Related to the `optuna.samplers` submodule. This is automatically labeled by github-actions.
Projects
None yet
Milestone
v3.1.0
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@c-bata @codecov-commenter @contramundum53 @gen740