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

Display best objective value in contour plot for a given param pair, not the value from the most recent trial #4848

Merged
merged 2 commits into from
Aug 3, 2023

Conversation

xadrianzetx
Copy link
Collaborator

Motivation

This patch changes behavior of contour plot (both backends) when two or more trials params are overlapping. Closes #4733.

Description of the changes

Currently, the objective value always comes from the last completed trial. This implementation selects the best objective value w.r.t. optimization direction (with reasonable default in multi-objective cases).

This patch changes behavior of contour plot (both backends) when two or
more trials params are overlapping. Currently, the objective value always
comes from the last completed trial. This implementation selects the
best objective value wrt. optimization direction (with reasonable
default in multi-objective cases).
@github-actions github-actions bot added the optuna.visualization Related to the `optuna.visualization` submodule. This is automatically labeled by github-actions. label Jul 31, 2023
@not522
Copy link
Member

not522 commented Aug 1, 2023

@Alnusjaponica Could you review this PR?

Copy link
Member

@not522 not522 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 you PR! It's almost LGTM. Please check my comment.

Comment on lines 330 to 331
# In multi-objective case we don't know which objective was selected in target
# function therefore we don't know how to select the best objective value.
Copy link
Member

Choose a reason for hiding this comment

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

Since target is also used in single-objective optimization, could you please modify this comment? For example, other metrics or elapsed time may be used as target.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good point, updated in 4d7f407.

@not522 not522 added the feature Change that does not break compatibility, but affects the public interfaces. label Aug 2, 2023
@xadrianzetx
Copy link
Collaborator Author

I see the sphinx issue already fixed on main, so I'll leave it at that since this check is not required.

@codecov-commenter
Copy link

Codecov Report

Merging #4848 (4d7f407) into master (2f2636f) will increase coverage by 0.00%.
Report is 42 commits behind head on master.
The diff coverage is 100.00%.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

@@           Coverage Diff           @@
##           master    #4848   +/-   ##
=======================================
  Coverage   89.54%   89.54%           
=======================================
  Files         197      199    +2     
  Lines       14676    14740   +64     
=======================================
+ Hits        13141    13199   +58     
- Misses       1535     1541    +6     
Files Changed Coverage Δ
optuna/visualization/_contour.py 97.22% <100.00%> (+0.05%) ⬆️

... and 10 files with indirect coverage changes

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

Copy link
Member

@not522 not522 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! LGTM!

@not522 not522 removed their assignment Aug 3, 2023
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!
@not522 Could you merge this PR?

@Alnusjaponica Alnusjaponica removed their assignment Aug 3, 2023
@not522 not522 merged commit 19a1ab5 into optuna:master Aug 3, 2023
30 of 31 checks passed
@not522 not522 added this to the v3.3.0 milestone Aug 3, 2023
@xadrianzetx xadrianzetx deleted the contour-best-values branch August 3, 2023 08:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature Change that does not break compatibility, but affects the public interfaces. optuna.visualization Related to the `optuna.visualization` submodule. This is automatically labeled by github-actions.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Display best objective value in contour plot for a given param pair, not the value from the most recent trial
4 participants