Skip to content

Create optuna.visualization.matplotlib.#1756

Merged
HideakiImamura merged 14 commits intooptuna:masterfrom
ytknzw:matplotlib_api
Sep 12, 2020
Merged

Create optuna.visualization.matplotlib.#1756
HideakiImamura merged 14 commits intooptuna:masterfrom
ytknzw:matplotlib_api

Conversation

@ytknzw
Copy link
Contributor

@ytknzw ytknzw commented Aug 30, 2020 •
edited
Loading

This PR proposes to implement matplotlib backend for plotting functions as optuna.visualization.matplotlib.

Motivation

To provide another visualization option for users, who have only plotly now.
See issue #1539 and PR #1707 for the original motivation.

Description of the changes

Create optuna.visualization.matplotlib. All the functions corresponding to the current Plotly backend are to be implemented in PR #1707 the following PRs.

@github-actions github-actions bot added the optuna.visualization Related to the `optuna.visualization` submodule. This is automatically labeled by github-actions. label Aug 30, 2020
@HideakiImamura HideakiImamura self-assigned this Aug 30, 2020
@HideakiImamura HideakiImamura added the feature Change that does not break compatibility, but affects the public interfaces. label Aug 30, 2020
HideakiImamura
HideakiImamura reviewed Aug 30, 2020
View reviewed changes
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! I have several comments. Could you take a look?

@c-bata
Copy link
Member

c-bata commented Aug 30, 2020

How about retuning matplotlib's Axes object like pandas.DataFrame.plot() function. It makes more customizable (related discussion is #704).

https://pandas.pydata.org/pandas-docs/version/0.23.4/generated/pandas.DataFrame.plot.html

@c-bata
Copy link
Member

c-bata commented Aug 30, 2020 •
edited
Loading

Some approaches for how we could go about designing the public API.

  1. Introduce a “backend” parameter to existing visualization functions.
    • User must consider the argument in each call, implementation becomes somewhat complicated, and the return value becomes a Union.
  2. Define a global/thread local state.
    • User only have to configure it once. Other cons remain. We introduce a global state.
  3. Introduce optuna.visualization.matplotlib to define plot_*
    • User switched between backends by changing the top import statement(s). Pro that we don’t have to keep a global state, code is clearly separated. What functions are implemented in what backend becomes very clear. Return value is not a Union.

I would suggest 3., as you might have noticed. And if so, changes required from this PR are not that large and we could incorporate them here.

In the viewpoint of #1707 (comment), I'm +1 to introduce optuna.visualization.matplotlib like this pull request 👍

ytknzw and others added 3 commits August 30, 2020 16:22
@ytknzw
Copy link
Contributor Author

ytknzw commented Aug 30, 2020

@HideakiImamura @c-bata
Thank you for your reviews and comments!
I've made some changes to the codes.

@ytknzw
Copy link
Contributor Author

ytknzw commented Aug 30, 2020

@c-bata
#1756 (comment)

How about retuning matplotlib's Axes object like pandas.DataFrame.plot() function.

Thank you for your suggestion. I assume you mean 'return' not 'retune', right?
My current Matplotlib implementation returns Figure object. For compatibility with the Plotly implementation, I'll work on this in PR #1707 as well.

This was referenced Aug 30, 2020
Closed
Merged
Merged
Merged
Merged
Merged
Merged
@ytknzw ytknzw mentioned this pull request Sep 12, 2020
Merged
@c-bata
Copy link
Member

c-bata commented Sep 12, 2020 •
edited
Loading

Sorry for my delayed response 🙏

Thank you for your suggestion. I assume you mean 'return' not 'retune', right?

Exactly! It was just a typo of 'returning'.

My current Matplotlib implementation returns Figure object. For compatibility with the Plotly implementation, I'll work on this in PR #1707 as well.

IMO, it's better to type returned value to matplotlib.Axes in this pull request to avoid interface breaking changes.
What do you think?

@toshihikoyanase
Copy link
Member

toshihikoyanase commented Sep 12, 2020

How about retuning matplotlib's Axes object like pandas.DataFrame.plot() function.

Sounds good. When we released the current visualization functions, some users requested to customize the figures. IMO, it is reasonable to provide such a feature for matplotlib backend.

@ytknzw
Could you change the return type of plot_xxx in this PR, too?

@ytknzw
Copy link
Contributor Author

ytknzw commented Sep 12, 2020

@c-bata @toshihikoyanase
Thank you for your suggestions. Yes, I agree with you.
Just changed the return value type to Axes.

HideakiImamura
HideakiImamura approved these changes Sep 12, 2020
View reviewed changes
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.

LGTM!

c-bata
c-bata approved these changes Sep 12, 2020
View reviewed changes
Copy link
Member

@c-bata c-bata left a comment

Choose a reason for hiding this comment

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

LGTM after passed CI 👍

toshihikoyanase
toshihikoyanase requested changes Sep 12, 2020
View reviewed changes
Copy link
Member

@toshihikoyanase toshihikoyanase 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 your update. I added some small comments. I may be able to work on them as a follow-up task.

ytknzw and others added 4 commits September 12, 2020 17:23
@ytknzw
Merge remote-tracking branch 'upstream/master' into matplotlib_api
5e409f0
@ytknzw @toshihikoyanase
Update optuna/visualization/matplotlib/_utils.py
f9e76a1 
Co-authored-by: Toshihiko Yanase <toshihiko.yanase@gmail.com>
@ytknzw @toshihikoyanase
Update optuna/visualization/matplotlib/_utils.py
0db54af 
Co-authored-by: Toshihiko Yanase <toshihiko.yanase@gmail.com>
@ytknzw @toshihikoyanase
Update optuna/visualization/matplotlib/_utils.py
facb83b 
Co-authored-by: Toshihiko Yanase <toshihiko.yanase@gmail.com>
toshihikoyanase
toshihikoyanase approved these changes Sep 12, 2020
View reviewed changes
Copy link
Member

@toshihikoyanase toshihikoyanase 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 your swift action. LGTM. I'm waiting for the CI jobs to merge this PR.

@HideakiImamura HideakiImamura merged commit 1549364 into optuna:master Sep 12, 2020
@HideakiImamura HideakiImamura added this to the v2.2.0 milestone Sep 12, 2020
@ytknzw ytknzw mentioned this pull request Sep 14, 2020
Merged
@ytknzw ytknzw deleted the matplotlib_api branch September 14, 2020 07:22
@keisuke-umezawa keisuke-umezawa mentioned this pull request Sep 25, 2020
Merged
15 tasks
@ytknzw ytknzw mentioned this pull request Nov 3, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@toshihikoyanase toshihikoyanase toshihikoyanase approved these changes

@c-bata c-bata c-bata approved these changes

@HideakiImamura HideakiImamura HideakiImamura approved these changes

Assignees

@HideakiImamura HideakiImamura

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

Milestone

v2.2.0

Development

Successfully merging this pull request may close these issues.

4 participants

@ytknzw @c-bata @toshihikoyanase @HideakiImamura