Merge pull request #740 from nabenabe0928/bug-fix/display-conditional-params-properly

Display conditional parameters properly in TrialTable

optuna_dashboard/ts/components/DataGrid.tsx

@@ -35,7 +35,7 @@ interface DataGridColumn<T> {
   label: string
   sortable?: boolean
   less?: (a: T, b: T, ascending: boolean) => number
-  filterChoices?: string[]
+  filterChoices?: (string | null)[]
   toCellValue?: (rowIndex: number) => string | React.ReactNode
   padding?: "normal" | "checkbox" | "none"
 }
@@ -333,7 +333,7 @@ function DataGridHeaderColumn<T>(props: {
                       <CheckBoxOutlineBlankIcon color="primary" />
                     )}
                   </ListItemIcon>
-                  {choice}
+                  {choice ?? "(missing value)"}
                 </MenuItem>
               ))}
             </Menu>

optuna_dashboard/ts/components/TrialTable.tsx

@@ -25,29 +25,37 @@ export const TrialTable: FC<{
       toCellValue: (i) => trials[i].state.toString(),
     },
   ]
+  const valueComparator = (
+    firstVal?: TrialValueNumber,
+    secondVal?: TrialValueNumber,
+    ascending: boolean = true
+  ): number => {
+    if (firstVal === secondVal) {
+      return 0
+    }
+    if (firstVal === undefined) {
+      return ascending ? -1 : 1
+    } else if (secondVal === undefined) {
+      return ascending ? 1 : -1
+    }
+    if (firstVal === "-inf" || secondVal === "inf") {
+      return 1
+    } else if (secondVal === "-inf" || firstVal === "inf") {
+      return -1
+    }
+    return firstVal < secondVal ? 1 : -1
+  }
   if (studyDetail === null || studyDetail.directions.length === 1) {
     columns.push({
       field: "values",
       label: "Value",
       sortable: true,
       less: (firstEl, secondEl, ascending): number => {
-        const firstVal = firstEl.values?.[0]
-        const secondVal = secondEl.values?.[0]
-
-        if (firstVal === secondVal) {
-          return 0
-        }
-        if (firstVal === undefined) {
-          return ascending ? -1 : 1
-        } else if (secondVal === undefined) {
-          return ascending ? 1 : -1
-        }
-        if (firstVal === "-inf" || secondVal === "inf") {
-          return 1
-        } else if (secondVal === "-inf" || firstVal === "inf") {
-          return -1
-        }
-        return firstVal < secondVal ? 1 : -1
+        return valueComparator(
+          firstEl.values?.[0],
+          secondEl.values?.[0],
+          ascending
+        )
       },
       toCellValue: (i) => {
         if (trials[i].values === undefined) {
@@ -66,23 +74,11 @@ export const TrialTable: FC<{
             : `Objective ${objectiveId}`,
         sortable: true,
         less: (firstEl, secondEl, ascending): number => {
-          const firstVal = firstEl.values?.[objectiveId]
-          const secondVal = secondEl.values?.[objectiveId]
-
-          if (firstVal === secondVal) {
-            return 0
-          }
-          if (firstVal === undefined) {
-            return ascending ? -1 : 1
-          } else if (secondVal === undefined) {
-            return ascending ? 1 : -1
-          }
-          if (firstVal === "-inf" || secondVal === "inf") {
-            return 1
-          } else if (secondVal === "-inf" || firstVal === "inf") {
-            return -1
-          }
-          return firstVal < secondVal ? 1 : -1
+          return valueComparator(
+            firstEl.values?.[objectiveId],
+            secondEl.values?.[objectiveId],
+            ascending
+          )
         },
         toCellValue: (i) => {
           if (trials[i].values === undefined) {
@@ -93,55 +89,41 @@ export const TrialTable: FC<{
       }))
     columns.push(...objectiveColumns)
   }
-  if (
-    studyDetail?.union_search_space.length ===
+  const isDynamicSpace =
+    studyDetail?.union_search_space.length !==
     studyDetail?.intersection_search_space.length
-  ) {
-    studyDetail?.intersection_search_space.forEach((s) => {
-      const sortable = s.distribution.type !== "CategoricalDistribution"
-      const filterChoices =
-        s.distribution.type === "CategoricalDistribution"
-          ? s.distribution.choices.map((c) => c.value)
-          : undefined
-      columns.push({
-        field: "params",
-        label: `Param ${s.name}`,
-        toCellValue: (i) =>
-          trials[i].params.find((p) => p.name === s.name)
-            ?.param_external_value || null,
-        sortable: sortable,
-        filterChoices: filterChoices,
-        // eslint-disable-next-line @typescript-eslint/no-unused-vars
-        less: (firstEl, secondEl, _): number => {
-          const firstVal = firstEl.params.find(
-            (p) => p.name === s.name
-          )?.param_internal_value
-          const secondVal = secondEl.params.find(
-            (p) => p.name === s.name
-          )?.param_internal_value
-
-          if (firstVal === secondVal) {
-            return 0
-          } else if (firstVal && secondVal) {
-            return firstVal < secondVal ? 1 : -1
-          } else if (firstVal) {
-            return -1
-          } else {
-            return 1
-          }
-        },
-      })
-    })
-  } else {
+  studyDetail?.union_search_space.forEach((s) => {
+    const sortable = s.distribution.type !== "CategoricalDistribution"
+    const filterChoices: (string | null)[] | undefined =
+      s.distribution.type === "CategoricalDistribution"
+        ? s.distribution.choices.map((c) => c.value)
+        : undefined
+    const hasMissingValue = trials.some(
+      (t) => !t.params.some((p) => p.name === s.name)
+    )
+    if (filterChoices !== undefined && isDynamicSpace && hasMissingValue) {
+      filterChoices.push(null)
+    }
     columns.push({
       field: "params",
-      label: "Params",
+      label: `Param ${s.name}`,
       toCellValue: (i) =>
-        trials[i].params
-          .map((p) => p.name + ": " + p.param_external_value)
-          .join(", "),
+        trials[i].params.find((p) => p.name === s.name)?.param_external_value ||
+        null,
+      sortable: sortable,
+      filterChoices: filterChoices,
+      // eslint-disable-next-line @typescript-eslint/no-unused-vars
+      less: (firstEl, secondEl, _): number => {
+        const firstVal = firstEl.params.find(
+          (p) => p.name === s.name
+        )?.param_internal_value
+        const secondVal = secondEl.params.find(
+          (p) => p.name === s.name
+        )?.param_internal_value
+        return valueComparator(firstVal, secondVal)
+      },
     })
-  }
+  })
 
   studyDetail?.union_user_attrs.forEach((attr_spec) => {
     columns.push({
@@ -153,22 +135,16 @@ export const TrialTable: FC<{
       sortable: attr_spec.sortable,
       // eslint-disable-next-line @typescript-eslint/no-unused-vars
       less: (firstEl, secondEl, _): number => {
-        const firstVal = firstEl.user_attrs.find(
+        const firstValString = firstEl.user_attrs.find(
           (attr) => attr.key === attr_spec.key
         )?.value
-        const secondVal = secondEl.user_attrs.find(
+        const secondValString = secondEl.user_attrs.find(
           (attr) => attr.key === attr_spec.key
         )?.value
-
-        if (firstVal === secondVal) {
-          return 0
-        } else if (firstVal && secondVal) {
-          return Number(firstVal) < Number(secondVal) ? 1 : -1
-        } else if (firstVal) {
-          return -1
-        } else {
-          return 1
-        }
+        return valueComparator(
+          Number(firstValString) ?? firstValString,
+          Number(secondValString) ?? secondValString
+        )
       },
     })
   })