Merge pull request #617 from moririn2528/feedback-update

update feedback screen so it does not work by undoing history

optuna_dashboard/_app.py

@@ -46,6 +46,7 @@ from .artifact._backend import register_artifact_route
 from .artifact._backend_to_store import to_artifact_store
 from .preferential._study import _SYSTEM_ATTR_PREFERENTIAL_STUDY
 from .preferential._study import get_best_trials as get_best_preferential_trials
+from .preferential._system_attrs import get_skipped_trial_ids
 from .preferential._system_attrs import report_skip
 
 
@@ -220,6 +221,10 @@ def create_app(
         ) = get_cached_extra_study_property(study_id, trials)
 
         plotly_graph_objects = get_plotly_graph_objects(system_attrs)
+        trials_id2number = {trial._trial_id: trial.number for trial in trials}
+        skipped_trials = [
+            trials_id2number[trial_id] for trial_id in get_skipped_trial_ids(system_attrs)
+        ]
         return serialize_study_detail(
             summary,
             best_trials,
@@ -229,6 +234,7 @@ def create_app(
             union_user_attrs,
             has_intermediate_values,
             plotly_graph_objects,
+            skipped_trials,
         )
 
     @app.get("/api/studies/<study_id:int>/param_importances")

optuna_dashboard/_serializer.py

@@ -135,6 +135,7 @@ def serialize_study_detail(
     union_user_attrs: list[tuple[str, bool]],
     has_intermediate_values: bool,
     plotly_graph_objects: dict[str, str],
+    skipped_trials: list[int],
 ) -> dict[str, Any]:
     serialized: dict[str, Any] = {
         "name": summary.study_name,
@@ -166,6 +167,7 @@ def serialize_study_detail(
     if serialized["is_preferential"]:
         serialized["preference_history"] = serialize_preference_history(system_attrs)
         serialized["preferences"] = get_preferences(system_attrs)
+        serialized["skipped_trials"] = skipped_trials
     serialized["plotly_graph_objects"] = [
         {"id": id_, "graph_object": graph_object}
         for id_, graph_object in plotly_graph_objects.items()

optuna_dashboard/ts/apiClient.ts

@@ -56,24 +56,30 @@ const convertTrialResponse = (res: TrialResponse): Trial => {
 }
 
 interface PreferenceHistoryResponce {
-  id: string
-  preference_id: string
-  candidates: number[]
-  clicked: number
-  mode: PreferenceFeedbackMode
-  timestamp: string
+  history: {
+    id: string
+    preference_id: string
+    candidates: number[]
+    clicked: number
+    mode: PreferenceFeedbackMode
+    timestamp: string
+    preferences: [number, number][]
+  }
+  is_removed: boolean
 }
 
 const convertPreferenceHistory = (
   res: PreferenceHistoryResponce
 ): PreferenceHistory => {
   return {
-    id: res.id,
-    preference_id: res.preference_id,
-    candidates: res.candidates,
-    clicked: res.clicked,
-    feedback_mode: res.mode,
-    timestamp: new Date(res.timestamp),
+    id: res.history.id,
+    preference_id: res.history.preference_id,
+    candidates: res.history.candidates,
+    clicked: res.history.clicked,
+    feedback_mode: res.history.mode,
+    timestamp: new Date(res.history.timestamp),
+    preferences: res.history.preferences,
+    is_removed: res.is_removed,
   }
 }
 
@@ -95,6 +101,7 @@ interface StudyDetailResponse {
   preferences?: [number, number][]
   preference_history?: PreferenceHistoryResponce[]
   plotly_graph_objects: PlotlyGraphObject[]
+  skipped_trials?: number[]
 }
 
 export const getStudyDetailAPI = (
@@ -135,6 +142,7 @@ export const getStudyDetailAPI = (
           convertPreferenceHistory
         ),
         plotly_graph_objects: res.data.plotly_graph_objects,
+        skipped_trials: res.data.skipped_trials ?? [],
       }
     })
 }

optuna_dashboard/ts/components/PreferentialTrials.tsx

@@ -7,6 +7,7 @@ import {
   CardContent,
   CardActions,
   CardActionArea,
+  CircularProgress,
 } from "@mui/material"
 import ClearIcon from "@mui/icons-material/Clear"
 import IconButton from "@mui/material/IconButton"
@@ -111,7 +112,11 @@ const PreferentialTrial: FC<{
               padding: theme.spacing(2),
             }}
           >
-            <MarkdownRenderer body={trial.note.body} />
+            {trial.note.body !== "" ? (
+              <MarkdownRenderer body={trial.note.body} />
+            ) : (
+              <CircularProgress />
+            )}
           </Box>
 
           <ClearIcon
@@ -173,8 +178,8 @@ const PreferentialTrial: FC<{
 }
 
 type DisplayTrials = {
-  numbers: number[]
-  last_number: number
+  display: number[]
+  clicked: number[]
 }
 
 export const PreferentialTrials: FC<{ studyDetail: StudyDetail | null }> = ({
@@ -185,68 +190,87 @@ export const PreferentialTrials: FC<{ studyDetail: StudyDetail | null }> = ({
   }
   const theme = useTheme()
 
-  const runningTrials = studyDetail.trials.filter((t) => t.state === "Running")
-  const activeTrials = runningTrials.concat(studyDetail.best_trials)
+  const hiddenTrials = new Set(
+    studyDetail.preference_history
+      ?.map((p) => p.clicked)
+      .concat(studyDetail.skipped_trials) ?? []
+  )
+  const activeTrials = studyDetail.trials.filter(
+    (t) =>
+      (t.state === "Running" || t.state === "Complete") &&
+      !hiddenTrials.has(t.number)
+  )
 
   const [displayTrials, setDisplayTrials] = useState<DisplayTrials>({
-    numbers: activeTrials.map((t) => t.number),
-    last_number: Math.max(...activeTrials.map((t) => t.number), -1),
+    display: [],
+    clicked: [],
   })
-  const new_trails = activeTrials.filter(
+  const newTrials = activeTrials.filter(
     (t) =>
-      displayTrials.last_number < t.number &&
-      displayTrials.numbers.find((n) => n === t.number) === undefined
+      !displayTrials.display.includes(t.number) &&
+      !displayTrials.clicked.includes(t.number)
   )
-  if (new_trails.length > 0) {
-    setDisplayTrials((display) => {
-      const numbers = [...display.numbers]
-      new_trails.map((t) => {
-        const index = numbers.findIndex((n) => n === -1)
+  const deleteTrials = displayTrials.display.filter(
+    (t) => t !== -1 && !activeTrials.map((t) => t.number).includes(t)
+  )
+  if (newTrials.length > 0 || deleteTrials.length > 0) {
+    setDisplayTrials((prev) => {
+      const display = [...prev.display].map((t) =>
+        deleteTrials.includes(t) ? -1 : t
+      )
+      const clicked = [...prev.clicked]
+      newTrials.map((t) => {
+        const index = display.findIndex((n) => n === -1)
         if (index === -1) {
-          numbers.push(t.number)
+          display.push(t.number)
+          clicked.push(-1)
         } else {
-          numbers[index] = t.number
+          display[index] = t.number
         }
       })
       return {
-        numbers: numbers,
-        last_number: Math.max(...numbers, -1),
+        display: display,
+        clicked: clicked,
       }
     })
   }
 
   const hideTrial = (num: number) => {
-    setDisplayTrials((display) => {
-      const index = display.numbers.findIndex((n) => n === num)
+    setDisplayTrials((prev) => {
+      const index = prev.display.findIndex((n) => n === num)
       if (index === -1) {
-        return display
+        return prev
       }
-      const numbers = [...displayTrials.numbers]
-      numbers[index] = -1
+      const display = [...prev.display]
+      const clicked = [...prev.clicked]
+      display[index] = -1
+      clicked[index] = num
       return {
-        numbers: numbers,
-        last_number: display.last_number,
+        display: display,
+        clicked: clicked,
       }
     })
   }
 
   return (
     <Box padding={theme.spacing(2)}>
-      <Typography
-        variant="h4"
-        sx={{
-          marginBottom: theme.spacing(2),
-          fontWeight: theme.typography.fontWeightBold,
-        }}
-      >
-        Which trial is the worst?
-      </Typography>
+      <Box display="flex">
+        <Typography
+          variant="h4"
+          sx={{
+            marginBottom: theme.spacing(2),
+            fontWeight: theme.typography.fontWeightBold,
+          }}
+        >
+          Which trial is the worst?
+        </Typography>
+      </Box>
       <Box sx={{ display: "flex", flexDirection: "row", flexWrap: "wrap" }}>
-        {displayTrials.numbers.map((t, index) => (
+        {displayTrials.display.map((t, index) => (
           <PreferentialTrial
-            key={index}
+            key={t == -1 ? -index - 1 : t}
             trial={activeTrials.find((trial) => trial.number === t)}
-            candidates={displayTrials.numbers.filter((n) => n !== -1)}
+            candidates={displayTrials.display.filter((n) => n !== -1)}
             hideTrial={() => {
               hideTrial(t)
             }}

optuna_dashboard/ts/types/index.d.ts

@@ -206,6 +206,7 @@ type StudyDetail = {
   preferences?: [number, number][]
   preference_history?: PreferenceHistory[]
   plotly_graph_objects: PlotlyGraphObject[]
+  skipped_trials: number[]
 }
 
 type StudyDetails = {
@@ -222,4 +223,6 @@ type PreferenceHistory = {
   clicked: number
   feedback_mode: PreferenceFeedbackMode
   timestamp: Date
+  preferences: [number, number][]
+  is_removed: boolean
 }

python_tests/test_serializers.py

@@ -29,7 +29,9 @@ def test_get_study_detail_is_preferential() -> None:
     assert len(study_summaries) == 1
 
     study_summary = study_summaries[0]
-    study_detail = serialize_study_detail(study_summary, [], study.trials, [], [], [], False, {})
+    study_detail = serialize_study_detail(
+        study_summary, [], study.trials, [], [], [], False, {}, []
+    )
     assert study_detail["is_preferential"]
 
 
@@ -40,7 +42,9 @@ def test_get_study_detail_is_not_preferential() -> None:
     assert len(study_summaries) == 1
 
     study_summary = study_summaries[0]
-    study_detail = serialize_study_detail(study_summary, [], study.trials, [], [], [], False, {})
+    study_detail = serialize_study_detail(
+        study_summary, [], study.trials, [], [], [], False, {}, []
+    )
     assert not study_detail["is_preferential"]