-
-
Notifications
You must be signed in to change notification settings - Fork 971
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
Add timeline plot with matplotlib as backend #4538
Add timeline plot with matplotlib as backend #4538
Conversation
Codecov Report
📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more @@ Coverage Diff @@
## master #4538 +/- ##
==========================================
+ Coverage 90.81% 90.89% +0.08%
==========================================
Files 187 184 -3
Lines 13972 13954 -18
==========================================
- Hits 12688 12683 -5
+ Misses 1284 1271 -13
... and 40 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
@amylase @HideakiImamura Could you review this PR if you have time? |
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 the PR. I have several comments. PTAL.
By the way, can we check the plot appearance using the visual regression test? The CI job does not generate the appropriate link.
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 update the list of plot functions in the reference, please?
optuna/docs/source/reference/visualization/matplotlib.rst
Lines 13 to 21 in 2d0093a
optuna.visualization.matplotlib.plot_contour | |
optuna.visualization.matplotlib.plot_edf | |
optuna.visualization.matplotlib.plot_intermediate_values | |
optuna.visualization.matplotlib.plot_optimization_history | |
optuna.visualization.matplotlib.plot_parallel_coordinate | |
optuna.visualization.matplotlib.plot_param_importances | |
optuna.visualization.matplotlib.plot_pareto_front | |
optuna.visualization.matplotlib.plot_slice | |
optuna.visualization.matplotlib.is_available |
I have updated optuna/docs/source/reference/visualization/matplotlib.rst and fixed docstring in |
Oh, timeline_plot is not rendered... Maybe the reason is that optuna/optuna-visualization-regression-tests#8 is not merged yet. I have resolved comments in that PR. I think we can merge that PR now. (although there is still some room for follow-up) |
optuna/optuna-visualization-regression-tests#8 was merged! Let me re-render the plot. |
The timeline plots of Plotly backend were rendered as expected, but those of matplotlib backend weren't. This is because the CI checked out the current
I think we need #4555 to render plots using the code in PRs. https://642648d03da4640e33ad5d1b--effervescent-dieffenbachia-ab819a.netlify.app/plot_timeline.html |
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 the update. Almost LGTM. I have a minor comment. PTAL.
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.
Looks very good!
removed customized DateFormatter class added import matplotlib instead of dates, ticker, and patches
remove dates, patches, and ticker
Thank you for your update. Let me check again. |
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 for your update. Looks solid to me.
@@ -18,4 +18,5 @@ optuna.visualization.matplotlib | |||
optuna.visualization.matplotlib.plot_param_importances | |||
optuna.visualization.matplotlib.plot_pareto_front | |||
optuna.visualization.matplotlib.plot_slice | |||
optuna.visualization.matplotlib.plot_timeline |
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.
@amylase Let me merge this PR since it got two approvals. |
Motivation
cf: #4470
In addition to plotly, I want to implement timeline plot using matplotlib as backend.
Description of the changes
optuna/visualization/matplotlib/_timeline.py
.tests/visualization_tests/matplotlib_tests/test_timeline.py
.