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

Remove an obsoleted logic from CmaEsSampler #4239

Merged
merged 3 commits into from
Jan 17, 2023

Conversation

c-bata
Copy link
Member

@c-bata c-bata commented Dec 12, 2022

Motivation

Follow-up #4184. See #4184 (comment) for details.

Description of the changes

Remove the logic to keep the compatibility for Optuna v2.5 or older.

@c-bata c-bata added the code-fix Change that does not change the behavior, such as code refactoring. label Dec 12, 2022
@github-actions github-actions bot added the optuna.samplers Related to the `optuna.samplers` submodule. This is automatically labeled by github-actions. label Dec 12, 2022
@c-bata
Copy link
Member Author

c-bata commented Dec 12, 2022

@contramundum53 @eukaryo Could you review this PR?

@codecov-commenter
Copy link

codecov-commenter commented Dec 12, 2022

Codecov Report

Merging #4239 (5ce92bb) into master (dcbb316) will increase coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #4239      +/-   ##
==========================================
+ Coverage   90.42%   90.43%   +0.01%     
==========================================
  Files         172      172              
  Lines       13659    13657       -2     
==========================================
  Hits        12351    12351              
+ Misses       1308     1306       -2     
Impacted Files Coverage Δ
optuna/samplers/_cmaes.py 99.11% <100.00%> (-0.01%) ⬇️
optuna/integration/botorch.py 97.55% <0.00%> (+0.81%) ⬆️

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

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!

@c-bata c-bata changed the title Remove obsoleted logic from CmaEsSampler Remove an obsoleted logic from CmaEsSampler Dec 12, 2022
@contramundum53 contramundum53 removed their assignment Dec 16, 2022
@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 Dec 19, 2022
@c-bata c-bata added no-stale Exempt from stale bot and removed stale Exempt from stale bot labeling. labels Dec 26, 2022
@c-bata
Copy link
Member Author

c-bata commented Dec 26, 2022

Let me add no-stale label to avoid this unintentionally being closed.

@c-bata c-bata removed the no-stale Exempt from stale bot label Jan 12, 2023
@toshihikoyanase
Copy link
Member

@c-bata Could you resolve the conflict, please?
@eukaryo If you are busy, please feel free to remove the assignment.

@c-bata c-bata assigned HideakiImamura and unassigned eukaryo Jan 16, 2023
@c-bata
Copy link
Member Author

c-bata commented Jan 16, 2023

I resolved the file conflict. Let me reassign reviewers.

@HideakiImamura Could you review this PR if you have time?

@HideakiImamura
Copy link
Member

Thanks for the PR. The change looks good to me.

Let me ask one question. Does this change align with our deprecation policy? When we conduct breaking changes in the stable feature, we have a deprecation period over 2 major versions. As long as I can see, the CmaEsSampler has been stable from v2.0.0. Should we have a deprecation period with prior notice for users on this change?

@c-bata
Copy link
Member Author

c-bata commented Jan 17, 2023

@HideakiImamura Thank you for your review. That's a good point. It may be a bit controversial whether this change is considered as a breaking change.

First, there is no change in terms of the interface, so the user programs will not be broken. Second, in terms of optimization performance, there is a possibility that some user programs will perform worse. It is extremely rare cases like the following though.

  1. Created a study and executed an optimization using CMA-ES with Optuna v2.5 or prior
  2. Then, did not execute an optimization using CMA-ES with Optuna v2.6-v3.0
  3. Then, executed an optimization using CMA-ES of this PR.

I think this change is acceptable, but if you think it is a breaking change, this PR can be merged in v4.0 or v5.0. What do you think?

Copy link
Member

@HideakiImamura HideakiImamura 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 explanation. I agree with merging this PR in this release. LGTM.

@HideakiImamura HideakiImamura merged commit 45e441b into optuna:master Jan 17, 2023
@HideakiImamura HideakiImamura added this to the v3.1.0 milestone Jan 17, 2023
@c-bata c-bata deleted the cmaes-obsoleted-system-attrs branch January 17, 2023 05:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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
Development

Successfully merging this pull request may close these issues.

None yet

6 participants