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

Show tick labels and add log scale in slice graph #90

Merged
merged 6 commits into from
Apr 22, 2021

Conversation

simonhessner
Copy link
Contributor

Contributor License Agreement

This repository (optuna-dashboard) and Goptuna share common code.
This pull request may therefore be ported to Goptuna.
Make sure that you understand the consequences concerning licenses and check the box below if you accept the term before creating this pull request.

  • [ x] I agree this patch may be ported to Goptuna by other Goptuna contributors.

Reference Issues/PRs

No referenced issue as far as I know. I just found this a useful addition and I hope it helps other people too.

What does this implement/fix? Explain your changes.

There are two additions in the slice graph:

  1. The x axis now shows tick labels (for both numerical and categorical types)
  2. It is now possible to switch between log scale and linear scale

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.

Basically looks good to me. I left some review comments.

Comment on lines +191 to +193
tickfont: {
color: "#000000",
},
Copy link
Member

Choose a reason for hiding this comment

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

[nits] Could you tell me what is this attribute for?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This makes sure that the color of the x labels is black. It might not be strictly required, but since it was present in the non-numerical case, I added it here for consistency. If you want I can remove it in both places.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

By the way, there is a lot of duplicate code in the if(isnum) case and the else case. These cases basically only differ in the trace and the code could be simplyfied. I could do that if you want

Copy link
Contributor Author

@simonhessner simonhessner Apr 22, 2021

Choose a reason for hiding this comment

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

I have another question about the code. On line 177 you have xaxis: selected and on line 210 you have // xaxis: "paramName". So in one case (isnum) it is set and in the other case it is not. What is the effect of this parameter? I did not notice any difference when I commented it out, is it required?

Copy link
Member

Choose a reason for hiding this comment

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

This makes sure that the color of the x labels is black. It might not be strictly required, but since it was present in the non-numerical case, I added it here for consistency. If you want I can remove it in both places.

I see. You are right 👍

By the way, there is a lot of duplicate code in the if(isnum) case and the else case. These cases basically only differ in the trace and the code could be simplyfied. I could do that if you want

Sounds good! Welcome.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll make these changes in a separate PR because I don't know when I have time. Is everything in the current PR ok?

Copy link
Member

Choose a reason for hiding this comment

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

I'll make these changes in a separate PR because I don't know when I have time. Is everything in the current PR ok?

Sure!

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.

Thank you for your swift responses! LGTM 👍

@c-bata c-bata merged commit 567bc08 into optuna:main Apr 22, 2021
@simonhessner
Copy link
Contributor Author

Great, thank you! And thanks for this nice tool :-)

@c-bata c-bata added this to the v0.5.0 milestone Apr 22, 2021
@c-bata c-bata removed this from the v0.5.0 milestone Jun 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants