-
-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Support constraints for intermediate values plot #4851
Support constraints for intermediate values plot #4851
Conversation
Re. failing docs build, readthedocs/readthedocs.org#10279 seems relevant... Pinning the sphinx version to be less than 7 should fix this |
Thank you for your PR and the investigation for CI fail. The Optuna maintainers are working to fix the CI. The fix is not necessary to merge this PR, so don't worry about it. |
@Alnusjaponica Could you review this PR? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your contribution.
Could you make a similar change in optuna/visualization/matplotlib/_intermediate_values.py
?
marker={"maxdisplayed": 10}, | ||
marker=default_marker | ||
if tinfo.feasible | ||
else default_marker | {"color": "#CCCCCC"}, # type: ignore[operator] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using |
as a union operator for dict
is only available since Python 3.9
. Several ways to do it before Python 3.9
is mentioned in PEP584, so please use one of them.
@@ -69,7 +103,9 @@ def objective(trial: Trial, report_intermediate_values: bool) -> float: | |||
_IntermediatePlotInfo(trial_infos=[]), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By adding the following example, you'll see the test fails when python<=3.8
ref. #4851 (comment)
_IntermediatePlotInfo(
trial_infos=[
_TrialInfo(
trial_number=0, sorted_intermediate_values=[(0, 1.0), (1, 2.0)], feasible=True
),
_TrialInfo(
trial_number=1, sorted_intermediate_values=[(1, 2.0), (0, 1.0)], feasible=False
)
]
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you add this test case to confirm if test_plot_intermediate_values
works when study has infeasible results?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your prompt response!
I have added some comments and would appreciate it if you could review them.
marker={"maxdisplayed": 10}, | ||
marker=default_marker | ||
if tinfo.feasible | ||
else default_marker.update({"color": "#CCCCCC"}), # type: ignore[dict-item] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the confusion but dict.update()
returns None
like list.sort()
. Please change this part like this.
else default_marker.update({"color": "#CCCCCC"}), # type: ignore[dict-item] | |
else {**default_marker, "color": "#CCCCCC"}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Of course - silly mistake. Thank you :)
@@ -69,7 +103,9 @@ def objective(trial: Trial, report_intermediate_values: bool) -> float: | |||
_IntermediatePlotInfo(trial_infos=[]), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you add this test case to confirm if test_plot_intermediate_values
works when study has infeasible results?
Thanks! LGTM. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you! LGTM except the format. Please check the Checks
CI.
Codecov Report
❗ 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 #4851 +/- ##
==========================================
- Coverage 89.52% 89.48% -0.05%
==========================================
Files 197 200 +3
Lines 14676 14785 +109
==========================================
+ Hits 13139 13230 +91
- Misses 1537 1555 +18
... and 18 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! LGTM.
Motivation
References #4830
Description of the changes
Support constraints with the intermediate values plots, displaying infeasible trials in #CCCCCC colour.
Tests added accordingly.
Plot is below: