Fix bugs and add refactor changes

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 = {

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<boolean>(
-    () =>
-      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<HTMLButtonElement>): void => {
     e.preventDefault()
+    const values = widgetStates.map((ws) => ws.value)
     if (formWidgets.output_type == "objective") {
       const filtered = values.filter<number>((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 (
-                <TextInputWidget
-                  key={key}
-                  metricName={getMetricName(i)}
-                  widget={widget}
-                  widgetType={formWidgets.output_type}
-                  setValue={(value) => {
-                    setValue(i, value)
-                  }}
-                  value={value}
-                />
-              )
-            } else if (widget.type === "choice") {
-              return (
-                <FormControl key={key} sx={{ margin: theme.spacing(1, 2) }}>
-                  <FormLabel>
-                    {getMetricName(i)} - {widget.description}
-                  </FormLabel>
-                  <RadioGroup row defaultValue={widget.values.at(0)}>
-                    {widget.choices.map((c, j) => (
-                      <FormControlLabel
-                        key={c}
-                        control={
-                          <Radio
-                            checked={value === widget.values.at(j)}
-                            onChange={(e) => {
-                              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}
-                      />
-                    ))}
-                  </RadioGroup>
-                </FormControl>
-              )
-            } else if (widget.type === "slider") {
-              return (
-                <FormControl key={key} sx={{ margin: theme.spacing(1, 2) }}>
-                  <FormLabel>
-                    {getMetricName(i)} - {widget.description}
-                  </FormLabel>
-                  <Box sx={{ padding: theme.spacing(0, 2) }}>
-                    <Slider
-                      onChange={(e) => {
-                        // @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"
-                    />
-                  </Box>
-                </FormControl>
-              )
-            } else if (widget.type === "user_attr") {
-              return (
-                <FormControl key={key} sx={{ margin: theme.spacing(1, 2) }}>
-                  <FormLabel>{getMetricName(i)}</FormLabel>
-                  <TextField
-                    inputProps={{ readOnly: true }}
-                    value={value || undefined}
-                    error={value === null}
-                    helperText={
-                      value === null || value === undefined
-                        ? `This objective value is referred from trial.user_attrs[${widget.key}].`
-                        : ""
-                    }
-                  />
-                </FormControl>
-              )
-            }
-            return null
-          })}
+          {widgetStates.map((ws) => ws.render())}
           <Box
             sx={{
               display: "flex",
@@ -262,14 +163,22 @@ export const ObjectiveForm: FC<{
   )
 }
 
-const TextInputWidget: FC<{
-  widget: ObjectiveTextInputWidget
-  widgetType: "user_attr" | "objective"
+export const useTextInputWidget = (
+  key: string,
+  widgetType: "user_attr" | "objective",
+  widget: ObjectiveTextInputWidget,
   metricName: string
-  value: number | string
-  setValue: (value: number | string) => void
-}> = ({ widget, widgetType, metricName, value, setValue }) => {
+): WidgetState => {
   const theme = useTheme()
+  const [value, setValue] = useState<number | string>("")
+  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 (
-    <FormControl sx={{ margin: theme.spacing(1, 2) }}>
+  const render = () => (
+    <FormControl key={key} sx={{ margin: theme.spacing(1, 2) }}>
       <FormLabel>
         {metricName} - {widget.description}
       </FormLabel>
@@ -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<{
       />
     </FormControl>
   )
+  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<number>(widget.values[0])
+  const render = () => (
+    <FormControl key={key} sx={{ margin: theme.spacing(1, 2) }}>
+      <FormLabel>
+        {metricName} - {widget.description}
+      </FormLabel>
+      <RadioGroup row defaultValue={widget.values.at(0)}>
+        {widget.choices.map((c, j) => (
+          <FormControlLabel
+            key={c}
+            control={
+              <Radio
+                checked={value === widget.values.at(j)}
+                onChange={(e) => {
+                  const selected = widget.values.at(j)
+                  if (selected === undefined) {
+                    console.error("Must not reach here.")
+                    return
+                  }
+                  if (e.target.checked) {
+                    setValue(selected)
+                  }
+                }}
+              />
+            }
+            label={c}
+          />
+        ))}
+      </RadioGroup>
+    </FormControl>
+  )
+  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<number>(widget.min)
+  const render = () => (
+    <FormControl key={key} sx={{ margin: theme.spacing(1, 2) }}>
+      <FormLabel>
+        {metricName} - {widget.description}
+      </FormLabel>
+      <Box sx={{ padding: theme.spacing(0, 2) }}>
+        <Slider
+          onChange={(e) => {
+            // @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"
+        />
+      </Box>
+    </FormControl>
+  )
+  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 = () => (
+    <FormControl key={key} sx={{ margin: theme.spacing(1, 2) }}>
+      <FormLabel>{metricName}</FormLabel>
+      <TextField
+        inputProps={{ readOnly: true }}
+        value={value || ""}
+        error={value === null}
+        helperText={
+          value === null
+            ? `This objective value is referred from trial.user_attrs[${widget.key}].`
+            : ""
+        }
+      />
+    </FormControl>
+  )
+  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<{
                   <TextField
                     inputProps={{ readOnly: true }}
                     value={trial.values?.at(i)}
+                    disabled
                   />
                 </FormControl>
               )