Implement `log` argument for `suggest_int` of pycma integration. #1302

toshihikoyanase · 2020-05-29T00:36:22Z

Motivation

This one of the follow-up PRs of #1201.

Description of the changes

This PR adds support for IntLogUniformDistribution to optuna.integration.CmaEsSampler.

I have two concerns about this implementation:

Although optuna.samplers.CmaEsSampler quantizes the suggested values by step in the log domain, this implementation quantizes them in the linear domain similarly to the optuna.samplers.RandomSampler. If this change is acceptable, I think I also update the optuna.samplers.CmaEsSampler.
When optuna.samplers.RandomSampler suggests values from DiscreteUniformDistribution, it expands low and high by 0.5 * q to make the bins of quantization equivalent to others. On the other hand, it expands 0.5 when it uses IntLogUniformDistribution. I think this is because the low can be negative depending on step. In the most cases, users will use step=1 and the calculation will be correct. So, I follow this definition in optuna.integration.CmaEsSampler, but I'm a bit concerned about performance degradation when users specify step>1.

codecov-commenter · 2020-05-29T01:07:15Z

Codecov Report

Merging #1302 into master will increase coverage by 1.27%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #1302      +/-   ##
==========================================
+ Coverage   86.50%   87.78%   +1.27%     
==========================================
  Files          94       96       +2     
  Lines        7375     7317      -58     
==========================================
+ Hits         6380     6423      +43     
+ Misses        995      894     -101

Impacted Files	Coverage Δ
optuna/integration/cma.py	`93.53% <100.00%> (-0.59%)`	⬇️
optuna/samplers/cmaes.py	`75.92% <0.00%> (-1.24%)`	⬇️
optuna/storages/rdb/storage.py	`93.73% <0.00%> (-0.34%)`	⬇️
optuna/storages/base.py	`67.59% <0.00%> (-0.34%)`	⬇️
optuna/visualization/contour.py	`98.24% <0.00%> (-0.02%)`	⬇️
optuna/_experimental.py	`100.00% <0.00%> (ø)`
optuna/visualization/__init__.py	`100.00% <0.00%> (ø)`
optuna/visualization/parallel_coordinate.py	`100.00% <0.00%> (ø)`
optuna/visualization/param_importances.py	`100.00% <0.00%> (ø)`
optuna/_imports.py	`96.29% <0.00%> (ø)`
... and 33 more

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 e23ef65...946cbe3. Read the comment docs.

HideakiImamura

Thanks for the PR! I have some comments.

For the first your concern, I totally agree with you. I think we should fix optuna.samplers.CmaEsSampler as done in this PR.

HideakiImamura · 2020-06-01T04:33:31Z

optuna/integration/cma.py

+                # TODO(toshihikoyanase): Shiting 0.5 is not sufficient if step > 1.
+                lows.append(self._to_cma_params(search_space, param_name, dist.low - 0.5))
+                highs.append(self._to_cma_params(search_space, param_name, dist.high + 0.5))


Your second concern is reasonable: instead of expanding 0.5, we should expand 0.5*step. As you say, I think that IntLogUniformDistrbituion uses 0.5 to ensure low positive, but if we just want to ensure it, how about doing the following? I think the optuna.samplers.CmaEsSampler can be fixed as well.

Suggested change

# TODO(toshihikoyanase): Shiting 0.5 is not sufficient if step > 1.

lows.append(self._to_cma_params(search_space, param_name, dist.low - 0.5))

highs.append(self._to_cma_params(search_space, param_name, dist.high + 0.5))

# TODO(toshihikoyanase): Shiting 0.5 is not sufficient if step > 1.

lows.append(self._to_cma_params(search_space, param_name, max(0, dist.low - 0.5 * dist.step)))

highs.append(self._to_cma_params(search_space, param_name, dist.high + 0.5 * dist.step))

This change still has the problem of bias in discretization when low - 0.5 * step is negative. A possible solution is to move [low-0.5*step, high+0.5*step] parallel to [1, high - low + step + 1] for sampling when low - 0.5 * step is negative, and then undo it later. However, this is a change that has to be made to other samplers at the same time, and I think it's outside the scope of this PR.

I'm sorry for the delayed response.
Your new PR (#1329) will prohibit the simultaneous use of step and log, and we can keep the master's implementation. I'll remove the TODO comment about it.

optuna/integration/cma.py

HideakiImamura · 2020-06-17T02:47:08Z

Hi @toshihikoyanase! Is this PR ready for the review?

toshihikoyanase · 2020-06-23T00:43:22Z

@HideakiImamura I'm sorry for the delayed response. Regarding the step of IntLogUniformDistribution, #1329 ensures that the step is always 1, and I set the range to [low-0.5, high+0.5] similar to the other samplers like SkoptSampler.
Please take another look.

HideakiImamura

Thanks for the update! Basically, LGTM! I have some minor comments.

optuna/integration/cma.py

HideakiImamura

Thanks for the update. LGTM!

hvy

Thanks, basically LGTM.

hvy · 2020-06-30T03:56:21Z

optuna/integration/cma.py

+        if isinstance(dist, IntLogUniformDistribution):
+            exp_value = math.exp(cma_param_value)
+            r = numpy.round((exp_value - dist.low) / dist.step)
+            v = r * dist.step + dist.low


step has been removed (c.f. #1438) so I think we can safely assume it being 1.

optuna/integration/cma.py

hvy · 2020-06-30T04:03:50Z

optuna/integration/cma.py

-            elif isinstance(distribution, LogUniformDistribution):
+            elif isinstance(distribution, LogUniformDistribution) or isinstance(
+                distribution, IntLogUniformDistribution
+            ):


toshihikoyanase · 2020-06-30T06:54:58Z

I'm sorry but I pushed the wrong branch. I'm reverting the change.

Co-authored-by: Hideaki Imamura <38826298+HideakiImamura@users.noreply.github.com>

Remove redundant logic. Co-authored-by: Hideaki Imamura <38826298+HideakiImamura@users.noreply.github.com>

Co-authored-by: Hiroyuki Vincent Yamazaki <hiroyuki.vincent.yamazaki@gmail.com>

hvy · 2020-06-30T07:17:07Z

optuna/integration/cma.py

-                lows.append(dist.low - 0.5)
-                highs.append(dist.high + 0.5)
+                lows.append(dist.low - 0.5 * dist.step)
+                highs.append(dist.high + 0.5 * dist.step)


Just wondering but is this change somewhat orthogonal to the other?

You're right. It's the change for IntUniformDistribution. I reverted the change in 682e8f4. Thank you for your careful review.

Should we create a separate PR for that?

Thank you for your suggestion.
I created #1456. Please review it after this PR.

Thanks, I'll have a look at it.

hvy

Sorry for having it dragged out, LGTM!

Let me merge this PR after CI.

toshihikoyanase added enhancement Change that does not break compatibility and not affect public interfaces, but improves performance. optuna.integration Related to the `optuna.integration` submodule. This is automatically labeled by github-actions. labels May 29, 2020

HideakiImamura self-assigned this May 29, 2020

HideakiImamura reviewed Jun 1, 2020

View reviewed changes

nzw0301 reviewed Jun 1, 2020

View reviewed changes

optuna/integration/cma.py Outdated Show resolved Hide resolved

toshihikoyanase force-pushed the add-int-loguniform-to-pycma branch from 946cbe3 to 9f19cb6 Compare June 23, 2020 00:39

HideakiImamura reviewed Jun 23, 2020

View reviewed changes

optuna/integration/cma.py Outdated Show resolved Hide resolved

optuna/integration/cma.py Outdated Show resolved Hide resolved

toshihikoyanase force-pushed the add-int-loguniform-to-pycma branch from e6969bd to 709a677 Compare June 23, 2020 10:29

HideakiImamura approved these changes Jun 24, 2020

View reviewed changes

hvy self-assigned this Jun 30, 2020

hvy requested changes Jun 30, 2020

View reviewed changes

toshihikoyanase and others added 10 commits June 30, 2020 15:55

Add support for IntLogUniformDistribution to pycma sampler.

f6e16ab

Fix a typo.

826ffb2

Update optuna/integration/cma.py

411b939

Co-authored-by: Hideaki Imamura <38826298+HideakiImamura@users.noreply.github.com>

Remove an irrelevant TODO comment.

0e665df

Apply suggestions from code review

5e7fe41

Remove redundant logic. Co-authored-by: Hideaki Imamura <38826298+HideakiImamura@users.noreply.github.com>

Apply black.

dbf4c42

Fix variable name.

ec0d65d

Apply suggestions from code review

5916d3c

Co-authored-by: Hiroyuki Vincent Yamazaki <hiroyuki.vincent.yamazaki@gmail.com>

Remove the use of IntLogUniformDistribution.step.

95f5f0c

Reflect review comment on isinstance.

24dd979

toshihikoyanase force-pushed the add-int-loguniform-to-pycma branch from 423efb2 to 24dd979 Compare June 30, 2020 07:02

Apply refactoring of isinstance.

c268e3b

hvy reviewed Jun 30, 2020

View reviewed changes

Revert the change about IntUniformDistribution.

682e8f4

hvy approved these changes Jun 30, 2020

View reviewed changes

toshihikoyanase mentioned this pull request Jun 30, 2020

Use step to calculate range of IntUniformDistribution in PyCmaSampler. #1456

Merged

hvy merged commit 04adc0f into optuna:master Jun 30, 2020

hvy added this to the v2.0.0 milestone Jun 30, 2020

toshihikoyanase deleted the add-int-loguniform-to-pycma branch March 7, 2022 08:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement `log` argument for `suggest_int` of pycma integration. #1302

Implement `log` argument for `suggest_int` of pycma integration. #1302

toshihikoyanase commented May 29, 2020

codecov-commenter commented May 29, 2020 •

edited

HideakiImamura left a comment

HideakiImamura Jun 1, 2020

HideakiImamura Jun 1, 2020

toshihikoyanase Jun 8, 2020 •

edited

HideakiImamura commented Jun 17, 2020

toshihikoyanase commented Jun 23, 2020

HideakiImamura left a comment

HideakiImamura left a comment

hvy left a comment

hvy Jun 30, 2020

hvy Jun 30, 2020

toshihikoyanase commented Jun 30, 2020

hvy Jun 30, 2020

toshihikoyanase Jun 30, 2020

hvy Jun 30, 2020

toshihikoyanase Jun 30, 2020

hvy Jun 30, 2020

hvy left a comment

Implement log argument for suggest_int of pycma integration. #1302

Implement log argument for suggest_int of pycma integration. #1302

Conversation

toshihikoyanase commented May 29, 2020

Motivation

Description of the changes

codecov-commenter commented May 29, 2020 • edited

Codecov Report

HideakiImamura left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

toshihikoyanase Jun 8, 2020 • edited

Choose a reason for hiding this comment

HideakiImamura commented Jun 17, 2020

toshihikoyanase commented Jun 23, 2020

HideakiImamura left a comment

Choose a reason for hiding this comment

HideakiImamura left a comment

Choose a reason for hiding this comment

hvy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

toshihikoyanase commented Jun 30, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hvy left a comment

Choose a reason for hiding this comment

Implement `log` argument for `suggest_int` of pycma integration. #1302

Implement `log` argument for `suggest_int` of pycma integration. #1302

codecov-commenter commented May 29, 2020 •

edited

toshihikoyanase Jun 8, 2020 •

edited