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

Add x log scale for slice plot #120

Merged

Conversation

keisuke-umezawa
Copy link
Member

@keisuke-umezawa keisuke-umezawa commented Jul 15, 2021

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.

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

Reference Issues/PRs

To fix the #64

What does this implement/fix? Explain your changes.

Add switch for the log scale of x axis in addition to y axis.
スクリーンショット 2021-07-15 19 02 52

@keisuke-umezawa keisuke-umezawa changed the title [WIP] Add x log scale for slice plot, Add x log scale for slice plot, Jul 15, 2021
@@ -88,10 +94,18 @@ export const GraphSlice: FC<{
</Select>
</FormControl>
<FormControl component="fieldset" className={classes.formControl}>
<FormLabel component="legend">Log scale:</FormLabel>
<FormLabel component="legend">Log x scale:</FormLabel>
Copy link
Member Author

Choose a reason for hiding this comment

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

If you have a better name than Log x scale, please tell me that.

@keisuke-umezawa keisuke-umezawa changed the title Add x log scale for slice plot, Add x log scale for slice plot Jul 15, 2021
@c-bata c-bata added this to the v0.5.0 milestone Jul 15, 2021
@c-bata
Copy link
Member

c-bata commented Jul 16, 2021

Thank you for your pull request.
Let me assign @chenghuzi as a reviewer.

@chenghuzi
Copy link
Collaborator

LGTM! Just one thing: sometimes I need to click the switch twice to turn the log scale off or on. I noticed that we have the same issue in the log scale switch in the HistoryGraph. So probably we should remove the line e.preventDefault() in handleChange functions? It seems in the official doc this is also not necessary.

@c-bata
Copy link
Member

c-bata commented Jul 16, 2021

I think it's better to automatically determine x axis from Optuna's distribution instead of the toggle button. What do you think?

@chenghuzi
Copy link
Collaborator

I think it's better to automatically determine x axis from Optuna's distribution instead of the toggle button. What do you think?

Agreed. Could we just add an association between x axis scale and the type of distribution so that the graph can switch scale automatically?

@c-bata
Copy link
Member

c-bata commented Jul 20, 2021

Could we just add an association between x axis scale and the type of distribution so that the graph can switch scale automatically?

Yes. We can associate x axis scale with the distribution from intersection_search_space or union_search_space in the StudyDetailResponse so that the toggle button is no longer needed.

intersection_search_space: SearchSpace[]
union_search_space: SearchSpace[]

@c-bata
Copy link
Member

c-bata commented Jul 20, 2021

@keisuke-umezawa Can you work on it?

@keisuke-umezawa
Copy link
Member Author

I will!

@jeremycochoy
Copy link

Would this works well if the search space is dynamic and the variable was sampled both with log and uniform distribution?

@c-bata
Copy link
Member

c-bata commented Aug 2, 2021

Would this works well if the search space is dynamic and the variable was sampled both with log and uniform distribution?

Good point 👍 In such a case, I think it's enough just showing in linear scale.

@c-bata c-bata requested a review from chenghuzi August 2, 2021 09:17
@keisuke-umezawa keisuke-umezawa force-pushed the feature/add-x-log-scale-for-slice branch from 67d961b to b5e1c12 Compare August 9, 2021 07:28
@keisuke-umezawa
Copy link
Member Author

@chenghuzi I completed implementation. Could you review it?

Copy link
Collaborator

@chenghuzi chenghuzi left a comment

Choose a reason for hiding this comment

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

Overall LGTM! Just one thing, probably we can change the label of the log scale switch to specific Y axis.

@chenghuzi
Copy link
Collaborator

Overall LGTM! Thanks for your PR @keisuke-umezawa! I'll merge and close this.

@chenghuzi chenghuzi merged commit 75b9e78 into optuna:main Aug 30, 2021
@keisuke-umezawa keisuke-umezawa deleted the feature/add-x-log-scale-for-slice branch August 31, 2021 02:23
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.

4 participants