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

Reorder arguments of Trial.suggest_float. #1292

Merged
merged 2 commits into from
May 27, 2020

Conversation

toshihikoyanase
Copy link
Member

Motivation

This is a followup PR for #1201. It addresses this comment, and makes the order of the Trial.suggest_float arguments consistent with the Trial.suggest_int arguments. I think it is helpful for users if we include this change into v1.5.0.

Description of the changes

It reorders the argument of Trial.suggest_float. More concretely, it swaps log for step. This change does not affect user code because both of them are keyword-only arguments.

@toshihikoyanase toshihikoyanase added the code-fix Change that does not change the behavior, such as code refactoring. label May 26, 2020
@codecov-commenter
Copy link

Codecov Report

Merging #1292 into master will increase coverage by 0.05%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1292      +/-   ##
==========================================
+ Coverage   86.73%   86.78%   +0.05%     
==========================================
  Files          95       95              
  Lines        7438     7438              
==========================================
+ Hits         6451     6455       +4     
+ Misses        987      983       -4     
Impacted Files Coverage Δ
optuna/integration/chainermn.py 76.27% <100.00%> (+0.56%) ⬆️
optuna/multi_objective/trial.py 91.27% <100.00%> (ø)
optuna/trial/_base.py 58.62% <100.00%> (+1.72%) ⬆️
optuna/trial/_fixed.py 81.48% <100.00%> (+1.23%) ⬆️
optuna/trial/_trial.py 91.77% <100.00%> (+0.63%) ⬆️

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 77a550a...61733a3. Read the comment docs.

@hvy hvy assigned hvy and unassigned hvy May 27, 2020
@HideakiImamura HideakiImamura self-assigned this May 27, 2020
@hvy hvy self-assigned this May 27, 2020
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.

Thanks for picking up the TODO. The code changes LGTM but I'm wondering if we shouldn't update the order of the arguments in the corresponding documentation as well.

@toshihikoyanase
Copy link
Member Author

@hvy Thank you for your careful review. I reordered the arguments in the docstring for Trial.suggest_float. Fixed in 58ec8b5.

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.

Quick fix, thanks. LGTM!

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 PR! These changes are LGTM! I will merge this PR after CI checks.

@HideakiImamura HideakiImamura merged commit e7afd40 into optuna:master May 27, 2020
@toshihikoyanase toshihikoyanase added this to the v1.5.0 milestone May 29, 2020
@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
code-fix Change that does not change the behavior, such as code refactoring.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants