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

Modify simple examples based on the Optuna code conventions #218

Merged

Conversation

nabenabe0928
Copy link
Collaborator

Motivation

This PR refactors PR#216.
Primary changes in this PR is to modify the codes so that the examples will follow the Optuna code conventions.

Description of the changes

The typical code conventions are the following:

  1. Multi-objective function returns a tuple of objective values,
  2. Make the comments clearer without the context, e.g. the first line in the main block of multi_objective/quadratic_simple.py should use more general nouns like "the first objective" rather than obj1, obj2, and
  3. Specify the finished trial number if we use timeout.

@nabenabe0928 nabenabe0928 force-pushed the code-fix/refactor-simple-examples branch from 2953993 to f9272ee Compare October 19, 2023 07:29
@github-actions
Copy link

This pull request has not seen any recent activity.

@github-actions github-actions bot added the stale Exempt from stale bot labeling. label Oct 26, 2023
@toshihikoyanase
Copy link
Member

flake8 found some coding-style violations in quadratic_simple_constraint.py. Could you fix them, please?

Run flake8 .
./quadratic_simple_constraint.py:5:100: E501 line too long (102 > 99 characters)
./quadratic_simple_constraint.py:6:100: E501 line too long (101 > 99 characters)
./quadratic_simple_constraint.py:8:100: E501 line too long (105 > 99 characters)
./quadratic_simple_constraint.py:10:100: E501 line too long (109 > 99 characters)
./quadratic_simple_constraint.py:[11](https://github.com/optuna/optuna-examples/actions/runs/6571297355/job/17850176657?pr=218#step:6:12):100: E501 line too long (114 > 99 characters)
./quadratic_simple_constraint.py:[13](https://github.com/optuna/optuna-examples/actions/runs/6571297355/job/17850176657?pr=218#step:6:14):47: W291 trailing whitespace
./quadratic_simple_constraint.py:30:100: E501 line too long (108 > 99 characters)
6     E501 line too long (102 > 99 characters)
1     W291 trailing whitespace

@nabenabe0928
Copy link
Collaborator Author

flake8 found some coding-style violations in quadratic_simple_constraint.py. Could you fix them, please?

Run flake8 .
./quadratic_simple_constraint.py:5:100: E501 line too long (102 > 99 characters)
./quadratic_simple_constraint.py:6:100: E501 line too long (101 > 99 characters)
./quadratic_simple_constraint.py:8:100: E501 line too long (105 > 99 characters)
./quadratic_simple_constraint.py:10:100: E501 line too long (109 > 99 characters)
./quadratic_simple_constraint.py:[11](https://github.com/optuna/optuna-examples/actions/runs/6571297355/job/17850176657?pr=218#step:6:12):100: E501 line too long (114 > 99 characters)
./quadratic_simple_constraint.py:[13](https://github.com/optuna/optuna-examples/actions/runs/6571297355/job/17850176657?pr=218#step:6:14):47: W291 trailing whitespace
./quadratic_simple_constraint.py:30:100: E501 line too long (108 > 99 characters)
6     E501 line too long (102 > 99 characters)
1     W291 trailing whitespace

Thank you!
Done!

@github-actions github-actions bot removed the stale Exempt from stale bot labeling. label Oct 29, 2023
@not522
Copy link
Member

not522 commented Oct 30, 2023

@Alnusjaponica Could you review this PR?

Copy link
Collaborator

@Alnusjaponica Alnusjaponica left a comment

Choose a reason for hiding this comment

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

Almost LGTM, though I made some minor comments.
@toshihikoyanase Do you have any additional comments?

In this example, we optimize simple quadratic functions.
In this example, we optimize two objective values.
Unlike a single-objective optimization, an optimization gives the trade-off between two objectives.
As a result, we get best trade-offs between two objectives, a.k.a Pareto solutions.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I guess "the" is necessary in this case. Feel free to ignore this comment when you don't agree with it.

Suggested change
As a result, we get best trade-offs between two objectives, a.k.a Pareto solutions.
As a result, we get the best trade-offs between two objectives, a.k.a Pareto solutions.

print("All trials violated the constraints.")
print("Number of finished trials: ", len(study.trials))

feasible_trial_ids = [
Copy link
Collaborator

Choose a reason for hiding this comment

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

It might be better to avoid using the word id since trial.trial_id and trial.number are different. How about using other words such as numbers or idx instead.

@toshihikoyanase
Copy link
Member

@Alnusjaponica Thank you for your thorough review. I have no further comments, so please merge after your review.

@nabenabe0928 nabenabe0928 force-pushed the code-fix/refactor-simple-examples branch from de32944 to 3724cf4 Compare November 6, 2023 01:55
Copy link
Collaborator

@Alnusjaponica Alnusjaponica left a comment

Choose a reason for hiding this comment

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

LGTM

@Alnusjaponica Alnusjaponica merged commit 4416938 into optuna:main Nov 6, 2023
6 checks passed
@HideakiImamura HideakiImamura added this to the v3.5.0 milestone Dec 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants