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

Separate MOTPESampler from TPESampler #2616

Merged
merged 4 commits into from
May 20, 2021
Merged

Separate MOTPESampler from TPESampler #2616

merged 4 commits into from
May 20, 2021

Conversation

HideakiImamura
Copy link
Member

Depends on #2615.

Part of works for #2614.

Motivation

Currently, the MOTPESampler is defined to inherit from the TPESampler and is heavily influenced by changes made to the TPESampler. In order to phase in Issue #2614, this PR aims to make sure that the MOTPESampler is not affected by the TPESampler by copy-and-pasting some modules of TPESampler to MOTPESampler.

Description of the changes

  • Separate MOTPESampler from TPESampler

@HideakiImamura HideakiImamura added the enhancement Change that does not break compatibility and not affect public interfaces, but improves performance. label Apr 22, 2021
@github-actions github-actions bot added the optuna.samplers Related to the `optuna.samplers` submodule. This is automatically labeled by github-actions. label Apr 22, 2021
This was referenced Apr 22, 2021
Closed
Merged
@HideakiImamura HideakiImamura marked this pull request as draft April 22, 2021 07:34
@github-actions
Copy link
Contributor

github-actions bot commented May 9, 2021

This pull request has not seen any recent activity.

@github-actions github-actions bot added the stale Exempt from stale bot labeling. label May 9, 2021
Crissman
Crissman suggested changes May 11, 2021
View reviewed changes
Copy link
Contributor

@Crissman Crissman 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 PR. A first pass. 🙇

optuna/samplers/_tpe/multi_objective_sampler.py Outdated Show resolved Hide resolved
optuna/samplers/_tpe/multi_objective_sampler.py Outdated Show resolved Hide resolved
optuna/samplers/_tpe/multi_objective_sampler.py Outdated Show resolved Hide resolved
optuna/samplers/_tpe/multi_objective_sampler.py Outdated Show resolved Hide resolved
optuna/multi_objective/samplers/_motpe.py Outdated Show resolved Hide resolved
@HideakiImamura HideakiImamura marked this pull request as ready for review May 13, 2021 03:56
HideakiImamura
HideakiImamura commented May 13, 2021
View reviewed changes
Copy link
Member Author

@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.

@himkt @Crissman I've made the PR ready for review. PTAL.

optuna/samplers/_tpe/multi_objective_sampler.py
@@ -32,8 +34,129 @@ def _default_weights_above(x: int) -> np.ndarray:
return np.ones(x)


class _ParzenEstimatorParameters(
Copy link
Member Author

Choose a reason for hiding this comment

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

This class is copied from optuna.samplers._tpe.parzen_estimator._ParzenEstimatorParameters.

Copy link
Member

Choose a reason for hiding this comment

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

[just a question] Is it OK not to add some prefix for indicating that this parameter (and estimator) are for multi-objective TPE?

Copy link
Member

Choose a reason for hiding this comment

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

And is it needed to create a file for the estimator like _MultivariateParzenEstimator or _ParzenEstimatorParameters?

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for the comment. These classes are only being added temporarily and will be integrated as the #2614 task progresses. Therefore, I'm not too concerned about naming conventions here.

Copy link
Member

Choose a reason for hiding this comment

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

Mostly curious but could you possibly share more concretely how they will be merged together afterwards?

Copy link
Member Author

Choose a reason for hiding this comment

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

Now, I plan to entirely remove optuna/samplers/_tpe/multi_objectrive_sampler.py in the 4th task in #2614. _ParzenEstimatorParameters and _ParzenEstimator are edited in #2618. I will remove the current ones in multi_objective_sampler.py.

optuna/samplers/_tpe/multi_objective_sampler.py
pass


class _ParzenEstimator(object):
Copy link
Member Author

Choose a reason for hiding this comment

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

This class is copied from optuna.samplers._tpe.parzen_estimator._ParzenEstimator.

optuna/samplers/_tpe/multi_objective_sampler.py
@@ -576,6 +700,162 @@ def after_trial(

self._mo_random_sampler.after_trial(study, trial, state, values)

def _sample_from_gmm(
Copy link
Member Author

Choose a reason for hiding this comment

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

The following methods are copied from optuna.samplers._tpe.TPESampler.

@codecov-commenter
Copy link

Codecov Report

Merging #2616 (e6e590f) into master (be407fd) will decrease coverage by 0.05%.
The diff coverage is 86.92%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2616      +/-   ##
==========================================
- Coverage   91.68%   91.63%   -0.06%     
==========================================
  Files         138      138              
  Lines       11299    11445     +146     
==========================================
+ Hits        10360    10488     +128     
- Misses        939      957      +18
Impacted Files Coverage Δ
optuna/samplers/_tpe/multi_objective_sampler.py 92.16% <86.92%> (-3.62%) ⬇️
optuna/integration/botorch.py 98.65% <0.00%> (+0.89%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update be407fd...e6e590f. Read the comment docs.

@github-actions github-actions bot removed the stale Exempt from stale bot labeling. label May 13, 2021
himkt
himkt approved these changes May 17, 2021
View reviewed changes
Copy link
Member

@himkt himkt left a comment

Choose a reason for hiding this comment

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

LGTM

Crissman
Crissman approved these changes May 19, 2021
View reviewed changes
Copy link
Contributor

@Crissman Crissman 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 notes to explain where the code was copied from. LGTM!

hvy
hvy approved these changes May 20, 2021
View reviewed changes
Copy link
Member

@hvy hvy left a comment

Choose a reason for hiding this comment

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

LGTM!

@hvy hvy merged commit 203dc81 into optuna:master May 20, 2021
@hvy hvy added this to the v2.8.0 milestone May 20, 2021
@hvy hvy added code-fix Change that does not change the behavior, such as code refactoring. and removed enhancement Change that does not break compatibility and not affect public interfaces, but improves performance. labels May 20, 2021
@HideakiImamura HideakiImamura deleted the separate-mo-tpe branch June 9, 2023 02:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Crissman Crissman Crissman approved these changes

@himkt himkt himkt approved these changes

@hvy hvy hvy approved these changes

Assignees

@Crissman Crissman

@himkt himkt

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
v2.8.0
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@HideakiImamura @codecov-commenter @Crissman @himkt @hvy