-
-
Notifications
You must be signed in to change notification settings - Fork 985
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 rank plot matplotlib version #4541
Add rank plot matplotlib version #4541
Conversation
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
This pull request has not seen any recent activity. |
This pull request has not seen any recent activity. |
@Alnusjaponica Could you join the review, please? If you have any questions, please let me know. |
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.
I made some suggestions, some of which can possibly be addressed in different PRs. Feel free to ignore those minor changes if you don't agree with them.
) -> "Axes": | ||
"""Plot parameter relations as scatter plots with colors indicating ranks of objective value. | ||
|
||
Note that, if a parameter contains missing values, a trial with missing values is not plotted. |
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.
Note that, if a parameter contains missing values, a trial with missing values is not plotted. | |
Note that, trials with missing values will not be plotted. |
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 suggestion. It looks good change, but I think to apply this change in the follow-up PR because this change should also apply original plotly-version.
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.
"values" usually mean objective values, so maybe it's better to say "trials missing the specified parameters will not be plotted."
target: Optional[Callable[[FrozenTrial], float]] = None, | ||
target_name: str = "Objective Value", | ||
) -> "Axes": | ||
"""Plot parameter relations as scatter plots with colors indicating ranks of objective value. |
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.
"""Plot parameter relations as scatter plots with colors indicating ranks of objective value. | |
"""Plot parameter relations as scatter plots with colors indicating ranks of objective values. |
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.
I think that this change can also be resolved in the follow-up PR to re-write both of plotly version and matplotlib version.
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.
More precisely, this should be "ranks of target value".
Co-authored-by: Shinichi Hemmi <50256998+Alnusjaponica@users.noreply.github.com>
Co-authored-by: Shinichi Hemmi <50256998+Alnusjaponica@users.noreply.github.com>
Co-authored-by: Shinichi Hemmi <50256998+Alnusjaponica@users.noreply.github.com>
Co-authored-by: Shinichi Hemmi <50256998+Alnusjaponica@users.noreply.github.com>
Co-authored-by: Shinichi Hemmi <50256998+Alnusjaponica@users.noreply.github.com>
I added edgecolor to scatter plot as in #4602 . |
@Alnusjaponica Thank you for your review. I resolved most of comments and update my code to follow #4602. Could you take another look? |
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 my delayed reply. LGTM!
@contramundum53 Could you take another look?
if n_params == 0: | ||
_, ax = plt.subplots() | ||
ax.set_title(title) | ||
return ax | ||
if n_params == 1 or n_params == 2: | ||
fig, axs = plt.subplots() | ||
axs.set_title(title) | ||
pc = _add_rank_subplot(axs, sub_plot_infos[0][0]) | ||
else: | ||
fig, axs = plt.subplots(n_params, n_params) |
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.
Nit: Maybe we don't need these if
s and can directly have plt.subplots(len(sub_plot_infos), len(sub_plot_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.
I think these if
s are required by the following reason.
if n_params == 0:
is required because in this case,sub_plot_infos
is[]
andsub_plot_infos[0][0]
raises error if we remove thisif
.if n_params == 1 or n_params == 2:
is required becauseplt.subplots()
orplt.subplots(1, 1)
returnsAxes
object. On the other hand,plt.subplots(n, n)
returns array ofAxes
object ifn >= 2
. Thus,ax = axs[x_i, y_i]
raises error if we remove thisif
.
Otherwise, LGTM! |
I see, LGTM! |
Motivation
This is the follow-up PR of #4427 .
Description of the changes
Add
plot_rank
function inoptuna.visualization.matplotlib
.It is matplotlib version of
plot_rank
function inoptuna.visualization
which was added by #4427 .