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

Implement log argument for suggest_int of ChainerMN integration. #1275

Merged
merged 3 commits into from
May 26, 2020
Merged

Implement log argument for suggest_int of ChainerMN integration. #1275

merged 3 commits into from
May 26, 2020

Conversation

nzw0301
Copy link
Member

@nzw0301 nzw0301 commented May 22, 2020

Motivation

One of the follow-up PRs related to #1201

Description of the changes

  • Introduce log argument for chainermn integration.
  • Change low value to re-use test cases for suggest_int for simplicity.

@nzw0301
Copy link
Member Author

nzw0301 commented May 22, 2020

@himkt Could you review this PR?

@codecov-commenter
Copy link

codecov-commenter commented May 22, 2020

Codecov Report

Merging #1275 into master will decrease coverage by 0.17%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1275      +/-   ##
==========================================
- Coverage   86.90%   86.73%   -0.18%     
==========================================
  Files          94       95       +1     
  Lines        7299     7438     +139     
==========================================
+ Hits         6343     6451     +108     
- Misses        956      987      +31     
Impacted Files Coverage Δ
optuna/integration/chainermn.py 75.70% <ø> (+0.28%) ⬆️
optuna/cli.py 27.27% <0.00%> (-4.78%) ⬇️
optuna/storages/rdb/storage.py 95.16% <0.00%> (-0.67%) ⬇️
optuna/storages/base.py 67.92% <0.00%> (-0.30%) ⬇️
optuna/storages/cached_storage.py 95.63% <0.00%> (-0.20%) ⬇️
optuna/exceptions.py 100.00% <0.00%> (ø)
optuna/pruners/nop.py 75.00% <0.00%> (ø)
optuna/pruners/base.py 62.50% <0.00%> (ø)
optuna/samplers/base.py 47.05% <0.00%> (ø)
optuna/pruners/median.py 100.00% <0.00%> (ø)
... and 12 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 8125fa1...4cca60d. Read the comment docs.

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.

Thank you for your constant contribution, it looks good.

Copy link
Member

@toshihikoyanase toshihikoyanase left a comment

Choose a reason for hiding this comment

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

Thank you for your PR. It basically looks good to me.

It may be out of the scope of this PR, how about updating examples/chainermn_simple.py and examples/pruning/chainermn_integration.py?
I think we can simplify the suggestion of the number of units as follows:

- n_units = int(trial.suggest_loguniform("n_units_l{}".format(i), 4, 128))
+ n_units = trial.suggest_int("n_units_l{}".format(i), 4, 128, log=True)

tests/integration_tests/test_chainermn.py Outdated Show resolved Hide resolved
@toshihikoyanase toshihikoyanase self-assigned this May 26, 2020
@toshihikoyanase 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 26, 2020
nzw0301 and others added 2 commits May 26, 2020 16:29
Co-authored-by: Toshihiko Yanase <toshihiko.yanase@gmail.com>
@nzw0301
Copy link
Member Author

nzw0301 commented May 26, 2020

@toshihikoyanase Thank you for your comments! It does not seem to be out of the scope for me. I have included updates of examples you suggested in this PR.

Copy link
Member

@toshihikoyanase toshihikoyanase left a comment

Choose a reason for hiding this comment

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

Thank you for your update. LGTM!

@toshihikoyanase toshihikoyanase added this to the v1.5.0 milestone May 26, 2020
@toshihikoyanase toshihikoyanase merged commit 77a550a into optuna:master May 26, 2020
@nzw0301 nzw0301 deleted the suggest_int_log_chainermn branch May 26, 2020 13:06
@toshihikoyanase toshihikoyanase changed the title Implement log argument for suggest_int of chainermn Implement log argument for suggest_int of ChainerMN integration. May 27, 2020
@toshihikoyanase
Copy link
Member

Let me update the title for the release note.

@sile sile mentioned this pull request Jun 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants