From 63ac794d387808fd4eb99edd57ce4f37a69f6fa1 Mon Sep 17 00:00:00 2001 From: c-bata Date: Wed, 10 May 2023 00:28:09 +0900 Subject: [PATCH] Fix bugs and add refactor changes --- optuna_dashboard/_form_widget.py | 2 +- .../ts/components/ObjectiveForm.tsx | 376 ++++++++++-------- 2 files changed, 202 insertions(+), 176 deletions(-) diff --git a/optuna_dashboard/_form_widget.py b/optuna_dashboard/_form_widget.py index 045c3512..9517251a 100644 --- a/optuna_dashboard/_form_widget.py +++ b/optuna_dashboard/_form_widget.py @@ -216,7 +216,7 @@ def register_objective_form_widgets( ): warnings.warn("`user_attr_key` specified, but it will not be used.") if any( - isinstance(w, TextInputWidget) and w.optional is False for w in widgets + isinstance(w, TextInputWidget) and w.optional for w in widgets ): raise ValueError("TextInputWidget.optional must be False.") form_widgets: FormWidgetJSON = { diff --git a/optuna_dashboard/ts/components/ObjectiveForm.tsx b/optuna_dashboard/ts/components/ObjectiveForm.tsx index df2617af..74a08f18 100644 --- a/optuna_dashboard/ts/components/ObjectiveForm.tsx +++ b/optuna_dashboard/ts/components/ObjectiveForm.tsx @@ -1,4 +1,4 @@ -import React, { FC, useMemo, useState } from "react" +import React, { FC, ReactNode, useMemo, useState } from "react" import { Typography, Box, @@ -16,6 +16,12 @@ import { import { DebouncedInputTextField } from "./Debounce" import { actionCreator } from "../action" +type WidgetState = { + isValid: boolean + value: number | string + render: () => ReactNode +} + export const ObjectiveForm: FC<{ trial: Trial directions: StudyDirection[] @@ -24,61 +30,63 @@ export const ObjectiveForm: FC<{ }> = ({ trial, directions, names, formWidgets }) => { const theme = useTheme() const action = actionCreator() - const [values, setValues] = useState<(number | string)[]>( - formWidgets.widgets.map((widget) => { - if (widget.type === "text") { - return "" - } else if (widget.type === "choice") { - const value = widget.values.at(0) - if (value === undefined) { - console.error("Must not reach ehere") - return 0 - } - return value - } else if (widget.type === "slider") { - return widget.min - } else if (widget.type === "user_attr") { - const attr = trial.user_attrs.find((attr) => attr.key == widget.key) - if (attr === undefined) { - return 0 - } else { - const n = Number(attr.value) - return isNaN(n) ? 0 : n - } - } else { - console.error("Must not reach here") - return "" - } - }) - ) - const setValue = (objectiveId: number, value: number | string) => { - const newValues = [...values] - if (newValues.length <= objectiveId) { - return + const getMetricName = (i: number): string => { + if (formWidgets.output_type == "objective") { + if (names.at(i) !== undefined) { + return names[i] + } + return directions.length == 1 ? "Objective" : `Objective ${i}` + } else if (formWidgets.output_type == "user_attr") { + const key = formWidgets.widgets.at(i)?.user_attr_key + if (key !== undefined) { + return key + } } - newValues[objectiveId] = value - setValues(newValues) + console.error("Must not reach here") + return "Unknown" } + const widgetStates = formWidgets.widgets + .map((w, i) => { + const key = `${formWidgets.output_type}-${i}` + if (w.type === "text") { + return useTextInputWidget( + key, + formWidgets.output_type, + w, + getMetricName(i) + ) + } else if (w.type === "choice") { + return useChoiceWidget( + key, + formWidgets.output_type, + w, + getMetricName(i) + ) + } else if (w.type === "slider") { + return useSliderWidget( + key, + formWidgets.output_type, + w, + getMetricName(i) + ) + } else if (w.type === "user_attr") { + return useUserAttrRefWidget(key, w, getMetricName(i), trial) + } + console.error("Must not reach here") + return undefined + }) + .filter((w): w is WidgetState => w !== undefined) + const disableSubmit = useMemo( - () => - values.findIndex((v, i) => { - const w = formWidgets.widgets[i] - if ( - formWidgets.output_type === "user_attr" && - w.type === "text" && - w.optional - ) { - return false - } - return v === null - }) >= 0, - [values, formWidgets] + () => !widgetStates.every((ws) => ws.isValid), + [widgetStates] ) const handleSubmit = (e: React.MouseEvent): void => { e.preventDefault() + const values = widgetStates.map((ws) => ws.value) if (formWidgets.output_type == "objective") { const filtered = values.filter((v): v is number => v !== null) if (filtered.length !== directions.length) { @@ -96,23 +104,6 @@ export const ObjectiveForm: FC<{ } } - const getMetricName = (i: number): string => { - if (formWidgets.output_type == "objective") { - const n = names.at(i) - if (n !== undefined) { - return n - } - if (directions.length == 1) { - return `Objective` - } else { - return `Objective ${i}` - } - } else if (formWidgets.output_type == "user_attr") { - return formWidgets.widgets[i].user_attr_key as string - } - return "Unkown metric name" - } - const headerText = formWidgets.output_type === "user_attr" ? "Set User Attributes Form" @@ -138,97 +129,7 @@ export const ObjectiveForm: FC<{ p: theme.spacing(1), }} > - {formWidgets.widgets.map((widget, i) => { - const value = values.at(i) || "" - const key = `objective-${i}` - if (widget.type === "text") { - return ( - { - setValue(i, value) - }} - value={value} - /> - ) - } else if (widget.type === "choice") { - return ( - - - {getMetricName(i)} - {widget.description} - - - {widget.choices.map((c, j) => ( - { - const selected = widget.values.at(j) - if (selected === undefined) { - console.error("Must not reach here.") - } - if (e.target.checked) { - setValue(i, selected || 0) - } - }} - /> - } - label={c} - /> - ))} - - - ) - } else if (widget.type === "slider") { - return ( - - - {getMetricName(i)} - {widget.description} - - - { - // @ts-ignore - setValue(i, e.target.value as number) - }} - defaultValue={widget.min} - min={widget.min} - max={widget.max} - step={widget.step} - marks={ - widget.labels === null || widget.labels.length == 0 - ? true - : widget.labels - } - valueLabelDisplay="auto" - /> - - - ) - } else if (widget.type === "user_attr") { - return ( - - {getMetricName(i)} - - - ) - } - return null - })} + {widgetStates.map((ws) => ws.render())} void -}> = ({ widget, widgetType, metricName, value, setValue }) => { +): WidgetState => { const theme = useTheme() + const [value, setValue] = useState("") + const isValid = useMemo( + () => + widgetType === "user_attr" + ? value !== "" || widget.optional + : value !== "" && !isNaN(Number(value)), + [widget, value] + ) + const inputProps = widgetType === "objective" ? { @@ -278,9 +187,8 @@ const TextInputWidget: FC<{ : undefined const helperText = !widget.optional && value === "" ? `Please input the float number.` : "" - - return ( - + const render = () => ( + {metricName} - {widget.description} @@ -294,7 +202,7 @@ const TextInputWidget: FC<{ const n = Number(s) if (s.length > 0 && valid && !isNaN(n)) { setValue(n) - } else if (value === "") { + } else { setValue("") } }} @@ -310,6 +218,124 @@ const TextInputWidget: FC<{ /> ) + return { isValid, value, render } +} + +export const useChoiceWidget = ( + key: string, + widgetType: "user_attr" | "objective", + widget: ObjectiveChoiceWidget, + metricName: string +): WidgetState => { + const theme = useTheme() + const [value, setValue] = useState(widget.values[0]) + const render = () => ( + + + {metricName} - {widget.description} + + + {widget.choices.map((c, j) => ( + { + const selected = widget.values.at(j) + if (selected === undefined) { + console.error("Must not reach here.") + return + } + if (e.target.checked) { + setValue(selected) + } + }} + /> + } + label={c} + /> + ))} + + + ) + return { isValid: true, value, render } +} + +export const useSliderWidget = ( + key: string, + widgetType: "user_attr" | "objective", + widget: ObjectiveSliderWidget, + metricName: string +): WidgetState => { + const theme = useTheme() + const [value, setValue] = useState(widget.min) + const render = () => ( + + + {metricName} - {widget.description} + + + { + // @ts-ignore + setValue(e.target.value as number) + }} + defaultValue={widget.min} + min={widget.min} + max={widget.max} + step={widget.step} + marks={ + widget.labels === null || widget.labels.length == 0 + ? true + : widget.labels + } + valueLabelDisplay="auto" + /> + + + ) + return { isValid: true, value, render } +} + +export const useUserAttrRefWidget = ( + key: string, + widget: ObjectiveUserAttrRef, + metricName: string, + trial: Trial +): WidgetState => { + const theme = useTheme() + const value = useMemo(() => { + const attr = trial.user_attrs.find((attr) => attr.key === widget.key) + if (attr === undefined) { + return null + } + const n = Number(attr.value) + if (isNaN(n)) { + return null + } + return n + }, [trial.user_attrs]) + const render = () => ( + + {metricName} + + + ) + return { + isValid: value !== null, + value: value !== null ? value : "", + render, + } } export const ReadonlyObjectiveForm: FC<{ @@ -321,19 +347,18 @@ export const ReadonlyObjectiveForm: FC<{ const theme = useTheme() const getMetricName = (i: number): string => { if (formWidgets.output_type == "objective") { - const n = names.at(i) - if (n !== undefined) { - return n - } - if (directions.length == 1) { - return `Objective` - } else { - return `Objective ${i}` + if (names.at(i) !== undefined) { + return names[i] } + return directions.length == 1 ? "Objective" : `Objective ${i}` } else if (formWidgets.output_type == "user_attr") { - return formWidgets.widgets[i].user_attr_key as string + const key = formWidgets.widgets.at(i)?.user_attr_key + if (key !== undefined) { + return key + } } - return "Unkown metric name" + console.error("Must not reach here") + return "Unknown" } const getValue = (i: number): string | TrialValueNumber => { @@ -441,6 +466,7 @@ export const ReadonlyObjectiveForm: FC<{ )