Gather mask of None parameter in TPESampler #3886

knshnb · 2022-08-12T07:03:10Z

Motivation

Previously, in TPESampler, the mask of removing None parameters (= parameters that do not appear in the trial) was taken in two different places, which can easily cause bugs.

Description of the changes

Extract the corresponding logic from _build_observation_dict and _calculate_weights_below_for_multi_objective (as a consequence, build_observation_dict was removed).
Fix tests.

knshnb · 2022-08-16T05:26:51Z

@HideakiImamura Although I haven't modified the test yet, could you briefly check if this strategy is okay when you have time? By this PR, _sample_relative and sample_independent will get a little complex, but I'm thinking of DRYing them in another PR.

github-actions · 2022-08-23T23:05:22Z

This pull request has not seen any recent activity.

HideakiImamura · 2022-08-30T04:21:57Z

Thanks for the PR. I think this PR is good to reduce the complexity of TPESampler. Could you fix the CI errors?

knshnb · 2022-08-30T07:14:26Z

Thanks! I fixed the error. Could you take a look again and assign another reviewer?

codecov-commenter · 2022-08-30T07:27:23Z

Codecov Report

Merging #3886 (544c0cf) into master (97f90fc) will increase coverage by 0.02%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #3886      +/-   ##
==========================================
+ Coverage   90.09%   90.11%   +0.02%     
==========================================
  Files         162      162              
  Lines       12734    12742       +8     
==========================================
+ Hits        11473    11483      +10     
+ Misses       1261     1259       -2

Impacted Files	Coverage Δ
optuna/samplers/_tpe/sampler.py	`96.89% <100.00%> (-0.04%)`	⬇️
optuna/integration/chainer.py	`89.74% <0.00%> (ø)`
optuna/integration/__init__.py	`49.15% <0.00%> (ø)`
optuna/_hypervolume/__init__.py	`100.00% <0.00%> (ø)`
optuna/multi_objective/__init__.py	`100.00% <0.00%> (ø)`
optuna/importance/_fanova/__init__.py	`100.00% <0.00%> (ø)`
optuna/integration/allennlp/__init__.py	`100.00% <0.00%> (ø)`
optuna/multi_objective/samplers/__init__.py	`100.00% <0.00%> (ø)`
optuna/multi_objective/visualization/__init__.py	`100.00% <0.00%> (ø)`
optuna/study/_dataframe.py	`98.38% <0.00%> (+0.02%)`	⬆️
... and 6 more

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

github-actions · 2022-09-06T23:05:28Z

This pull request has not seen any recent activity.

c-bata · 2022-09-07T00:30:04Z

Let me reassign reviewers since this PR has no recent activities and Imamura-san will take a vacation.

@contramundum53 Could you review this PR?

github-actions · 2022-09-14T23:05:41Z

This pull request has not seen any recent activity.

contramundum53

LGTM!

github-actions · 2022-09-28T23:05:37Z

This pull request has not seen any recent activity.

c-bata

Sorry for the delayed review. I reviewed changes and leave some comments.

c-bata · 2022-10-03T11:53:03Z

optuna/samplers/_tpe/sampler.py

+        param_mask = ~np.isnan(list(config_values.values())[0])
+        param_mask_below, param_mask_above = param_mask[indices_below], param_mask[indices_above]
+        below = {k: v[indices_below[param_mask_below]] for k, v in config_values.items()}
+        above = {k: v[indices_above[param_mask_above]] for k, v in config_values.items()}


As written in L458, values variable is a dictionary that contains only a single hyperparameter, right?
If so, I'd say we can remove L458 and replace these lines like the following.

config_value = values["param_name"] param_mask = ~np.isnan(config_value) param_mask_below, param_mask_above = param_mask[indices_below], param_mask[indices_above] below = config_value[indices_below[param_mask_below]] above = config_value[indices_above[param_mask_above]]

What do you think?

Sure, I fixed it: 527d5fb.

c-bata · 2022-10-03T12:20:30Z

optuna/samplers/_tpe/sampler.py

+        # In the call of `sample_relative`, this logic makes sense because we only have the
+        # intersection search space or group decomposed search space. This means one parameter
+        # misses the one trial, then the other parameter must miss the trial, in this call of
+        # `sample_relative`.


This comment seems to have been added in #2688 to indicate that list(config_values.values())[0] is the correct logic.

But honestly, I don't understand what "This means one parameter misses the one trial, then the other parameter must miss the trial" means. Can you please tell me what kind of search space would include True in the param_mask? More specifically, what objective function would cause the this condition to be false?

Thanks for the comment. I investigated that issue and changed the previous behavior by #4079 to make it consistent with multivariate=False.

Now, that should happen in a dynamic search space when group=True.

github-actions · 2022-10-10T23:05:45Z

This pull request has not seen any recent activity.

knshnb · 2022-10-17T06:14:54Z

I figured out that the mask logic in sample_relative is unnecessary in the current code as pointed out by @c-bata. Let me make this PR draft until the above issue is fixed by #4079.

github-actions · 2022-10-25T23:05:27Z

This pull request has not seen any recent activity.

github-actions · 2022-11-08T23:05:38Z

This pull request was closed automatically because it had not seen any recent activity. If you want to discuss it, you can reopen it freely.

knshnb · 2022-11-11T08:48:30Z

Let me reopen this PR because the concern on the current implementation (link) by @c-bata has been resolved by #4079.

c-bata · 2022-11-11T08:51:49Z

Thanks! Let me assign @contramundum53 again.

contramundum53

The new code is easier to read for me. LGTM!

c-bata

LGTM:+1:

Gather mask of None parameter

dda0ee9

github-actions bot added the optuna.samplers Related to the `optuna.samplers` submodule. This is automatically labeled by github-actions. label Aug 12, 2022

HideakiImamura self-assigned this Aug 16, 2022

github-actions bot added the stale Exempt from stale bot labeling. label Aug 23, 2022

knshnb removed the stale Exempt from stale bot labeling. label Aug 30, 2022

knshnb added 2 commits August 30, 2022 15:41

Fix tests

02767b5

Move comments

efb6c88

knshnb changed the title ~~[WIP] Gather mask of None parameter in TPESampler~~ Gather mask of None parameter in TPESampler Aug 30, 2022

github-actions bot added the stale Exempt from stale bot labeling. label Sep 6, 2022

c-bata assigned c-bata and contramundum53 and unassigned HideakiImamura Sep 7, 2022

github-actions bot removed the stale Exempt from stale bot labeling. label Sep 7, 2022

github-actions bot added the stale Exempt from stale bot labeling. label Sep 14, 2022

contramundum53 approved these changes Sep 20, 2022

View reviewed changes

contramundum53 removed their assignment Sep 20, 2022

github-actions bot removed the stale Exempt from stale bot labeling. label Sep 20, 2022

github-actions bot added the stale Exempt from stale bot labeling. label Sep 28, 2022

c-bata removed the stale Exempt from stale bot labeling. label Sep 29, 2022

c-bata reviewed Oct 3, 2022

View reviewed changes

github-actions bot added the stale Exempt from stale bot labeling. label Oct 10, 2022

knshnb marked this pull request as draft October 17, 2022 06:15

github-actions bot removed the stale Exempt from stale bot labeling. label Oct 17, 2022

github-actions bot added the stale Exempt from stale bot labeling. label Oct 25, 2022

github-actions bot closed this Nov 8, 2022

Merge branch 'master' into refactor-tpe-none-mask

0412486

knshnb removed the stale Exempt from stale bot labeling. label Nov 11, 2022

knshnb reopened this Nov 11, 2022

knshnb added 2 commits November 11, 2022 17:43

Simplify mask logic in sample_independent

527d5fb

Update comment

544c0cf

knshnb marked this pull request as ready for review November 11, 2022 08:48

c-bata assigned contramundum53 Nov 11, 2022

contramundum53 approved these changes Nov 14, 2022

View reviewed changes

c-bata unassigned contramundum53 Nov 14, 2022

c-bata added the code-fix Change that does not change the behavior, such as code refactoring. label Nov 14, 2022

c-bata approved these changes Nov 16, 2022

View reviewed changes

c-bata added this to the v3.1.0 milestone Nov 16, 2022

c-bata merged commit dfb8d87 into optuna:master Nov 16, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Gather mask of None parameter in TPESampler #3886

Gather mask of None parameter in TPESampler #3886

knshnb commented Aug 12, 2022 •

edited by c-bata

Loading

knshnb commented Aug 16, 2022

github-actions bot commented Aug 23, 2022

HideakiImamura commented Aug 30, 2022

knshnb commented Aug 30, 2022

codecov-commenter commented Aug 30, 2022 •

edited

Loading

github-actions bot commented Sep 6, 2022

c-bata commented Sep 7, 2022

github-actions bot commented Sep 14, 2022

contramundum53 left a comment

github-actions bot commented Sep 28, 2022

c-bata left a comment

c-bata Oct 3, 2022

knshnb Nov 11, 2022

c-bata Oct 3, 2022

knshnb Nov 11, 2022

github-actions bot commented Oct 10, 2022

knshnb commented Oct 17, 2022

github-actions bot commented Oct 25, 2022

github-actions bot commented Nov 8, 2022

knshnb commented Nov 11, 2022

c-bata commented Nov 11, 2022

contramundum53 left a comment

c-bata left a comment

Gather mask of None parameter in TPESampler #3886

Gather mask of None parameter in TPESampler #3886

Conversation

knshnb commented Aug 12, 2022 • edited by c-bata Loading

Motivation

Description of the changes

knshnb commented Aug 16, 2022

github-actions bot commented Aug 23, 2022

HideakiImamura commented Aug 30, 2022

knshnb commented Aug 30, 2022

codecov-commenter commented Aug 30, 2022 • edited Loading

Codecov Report

github-actions bot commented Sep 6, 2022

c-bata commented Sep 7, 2022

github-actions bot commented Sep 14, 2022

contramundum53 left a comment

Choose a reason for hiding this comment

github-actions bot commented Sep 28, 2022

c-bata left a comment

Choose a reason for hiding this comment

c-bata Oct 3, 2022

Choose a reason for hiding this comment

knshnb Nov 11, 2022

Choose a reason for hiding this comment

c-bata Oct 3, 2022

Choose a reason for hiding this comment

knshnb Nov 11, 2022

Choose a reason for hiding this comment

github-actions bot commented Oct 10, 2022

knshnb commented Oct 17, 2022

github-actions bot commented Oct 25, 2022

github-actions bot commented Nov 8, 2022

knshnb commented Nov 11, 2022

c-bata commented Nov 11, 2022

contramundum53 left a comment

Choose a reason for hiding this comment

c-bata left a comment

Choose a reason for hiding this comment

knshnb commented Aug 12, 2022 •

edited by c-bata

Loading

codecov-commenter commented Aug 30, 2022 •

edited

Loading