Conversation
There was a problem hiding this comment.
Pull request overview
This PR refactors the experiment “Model Comparison” graphs UI to make metric selection more flexible (checkbox sidebar), improve empty-state handling, and align Plotly styling with the active MUI theme while integrating split selection with the parent view.
Changes:
- Reworked metric selection from the previous “general/custom” model into a checkbox-based metric filter sidebar.
- Updated chart data preparation to be split-aware (train/validation/test) and theme-aware (colors/layout).
- Adjusted the surrounding layout and SessionVisualization integration so the split selector drives both table and graphs.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| DashAI/front/src/pages/results/constants/layoutMaking.jsx | Refactors Plotly layout creation to use MUI theme colors and removes pie layout output. |
| DashAI/front/src/pages/results/constants/graphsMaking.jsx | Refactors trace building for radar/bar and introduces theme-based trace colors; removes pie traces. |
| DashAI/front/src/pages/results/components/ResultsGraphsSelection.jsx | Replaces button-based chart selection with an MUI ToggleButtonGroup. |
| DashAI/front/src/pages/results/components/ResultsGraphsPlot.jsx | Simplifies chart rendering and adds a “no metrics available” empty state. |
| DashAI/front/src/pages/results/components/ResultsGraphsParameters.jsx | Replaces previous metric selection controls with a metric checkbox sidebar + all/none actions. |
| DashAI/front/src/pages/results/components/ResultsGraphsLayout.jsx | Restructures layout to sidebar + chart area and updates the component interface accordingly. |
| DashAI/front/src/pages/results/components/ResultsGraphs.jsx | Rebuilds selection/state management around split + selected metrics; generates themed Plotly data/layout. |
| DashAI/front/src/components/models/SessionVisualization.jsx | Makes the split selector control both table and graph views; passes split state down to graphs. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const radarValues = values.map((v) => v ?? 0); | ||
| graphsToView.radar.push({ | ||
| type: "scatterpolar", | ||
| name: item.name, | ||
| automargin: true, | ||
| r: [...radarR, radarR[0]], // Add first value at the end to close the shape | ||
| theta: [...radarTheta, radarTheta[0]], // Add first label at the end | ||
| name: runLabel, | ||
| r: [...radarValues, radarValues[0]], | ||
| theta: [...metrics, metrics[0]], |
There was a problem hiding this comment.
Radar traces currently coerce missing metric values to 0 (v ?? 0). This can misrepresent “missing” as a real zero score and distort comparisons (especially for normalized metrics). Consider keeping missing values as null/NaN (so Plotly can break the polygon) or filtering out metrics that are missing for a given run.
| ResultsGraphsLayout.propTypes = { | ||
| selectedChart: PropTypes.string.isRequired, | ||
| handleChangeChart: PropTypes.func.isRequired, | ||
| showCustomMetrics: PropTypes.bool.isRequired, | ||
| handleToggleMetrics: PropTypes.func.isRequired, | ||
| tabularMetrics: PropTypes.array.isRequired, | ||
| selectedParameters: PropTypes.array.isRequired, | ||
| handleToggleParameter: PropTypes.func.isRequired, | ||
| selectedGeneralMetric: PropTypes.string.isRequired, | ||
| setSelectedGeneralMetric: PropTypes.func.isRequired, | ||
| setSelectedParameters: PropTypes.func.isRequired, | ||
| concatenatedMetrics: PropTypes.array.isRequired, | ||
| currentMetrics: PropTypes.array.isRequired, | ||
| selectedMetrics: PropTypes.array.isRequired, | ||
| handleToggleMetric: PropTypes.func.isRequired, | ||
| handleSelectAll: PropTypes.func.isRequired, | ||
| handleClearAll: PropTypes.func.isRequired, | ||
| chartData: PropTypes.object.isRequired, |
There was a problem hiding this comment.
ResultsGraphsLayout’s props were refactored (removed showCustomMetrics, selectedParameters, etc.), but there are still other components importing and rendering this layout with the old prop interface (e.g., front/src/components/pipelines/results/ResultsGraphs.jsx). This will now fail at runtime/prop validation and likely break the pipelines results view. Please update the other consumer(s) or provide a backward-compatible wrapper component.
| useEffect(() => { | ||
| setSelectedMetrics(availableMetrics[selectedSplit] ?? []); | ||
| }, [selectedSplit, availableMetrics]); |
There was a problem hiding this comment.
This effect resets selectedMetrics to all available metrics whenever availableMetrics changes (e.g., when runs updates), which can unexpectedly wipe out a user’s manual metric selection. Consider preserving the user selection by intersecting the previous selection with the newly available metrics, only auto-selecting defaults on initial load / when the split changes.
| train: Array.from(sets.train), | ||
| validation: Array.from(sets.validation), | ||
| test: Array.from(sets.test), |
There was a problem hiding this comment.
availableMetrics is built from Sets without sorting, so the metric ordering (and checkbox order / axis order) can vary depending on run iteration order. For stable UI and consistent comparisons, consider sorting the metric arrays (e.g., alphabetically) or using a deterministic ordering based on a canonical metric list.
| train: Array.from(sets.train), | |
| validation: Array.from(sets.validation), | |
| test: Array.from(sets.test), | |
| train: Array.from(sets.train).sort(), | |
| validation: Array.from(sets.validation).sort(), | |
| test: Array.from(sets.test).sort(), |
| }; | ||
| // Controlled or uncontrolled split | ||
| const selectedSplit = splitProp ?? internalSplit; | ||
| const handleChangeSplit = onSplitChange ?? setInternalSplit; |
There was a problem hiding this comment.
handleChangeSplit is computed but never used. If split selection is intentionally controlled exclusively by the parent (SessionVisualization), consider removing this unused variable/state to avoid confusion and keep the component focused.
| const handleChangeSplit = onSplitChange ?? setInternalSplit; |
| function layoutMaking(selectedChart, _graphsToView, theme) { | ||
| const bgColor = theme.palette.background.paper; | ||
| const textColor = theme.palette.text.primary; | ||
| const gridColor = theme.palette.divider; | ||
|
|
||
| // Layout only for Pie Charts | ||
| let numRows, numColumns; | ||
| if (graphsToView.pie.length <= 2) { | ||
| numRows = 1; | ||
| numColumns = graphsToView.pie.length; | ||
| } else { | ||
| numRows = Math.ceil(graphsToView.pie.length / 2); | ||
| numColumns = Math.min(2, graphsToView.pie.length); | ||
| } | ||
| const axisConfig = | ||
| selectedChart === "radar" | ||
| ? { | ||
| polar: { | ||
| bgcolor: bgColor, | ||
| radialaxis: { | ||
| visible: true, | ||
| gridcolor: gridColor, | ||
| linecolor: gridColor, | ||
| tickfont: { color: textColor }, | ||
| }, | ||
| angularaxis: { | ||
| color: textColor, | ||
| gridcolor: gridColor, | ||
| linecolor: gridColor, | ||
| }, | ||
| }, | ||
| } | ||
| : { | ||
| barmode: "group", | ||
| xaxis: { | ||
| gridcolor: gridColor, | ||
| zerolinecolor: gridColor, | ||
| tickfont: { color: textColor }, | ||
| }, | ||
| yaxis: { | ||
| gridcolor: gridColor, | ||
| zerolinecolor: gridColor, | ||
| tickfont: { color: textColor }, | ||
| }, | ||
| }; | ||
|
|
||
| const pieLayout = { | ||
| height: 480, | ||
| width: 800, | ||
| grid: { rows: numRows, columns: numColumns }, | ||
| const generalLayout = { | ||
| ...axisConfig, | ||
| showlegend: true, | ||
| height: 460, | ||
| autosize: true, | ||
| paper_bgcolor: bgColor, | ||
| plot_bgcolor: bgColor, | ||
| font: { | ||
| color: textColor, | ||
| family: "Quicksand-Bold, sans-serif", | ||
| size: 12, | ||
| }, | ||
| legend: { | ||
| itemclick: false, | ||
| bgcolor: bgColor, | ||
| bordercolor: gridColor, | ||
| borderwidth: 1, | ||
| }, | ||
| margin: { l: 60, r: 30, t: 40, b: 80 }, | ||
| }; | ||
|
|
||
| return { generalLayout, pieLayout }; | ||
| return { generalLayout }; |
There was a problem hiding this comment.
layoutMaking now requires a theme argument and no longer returns pieLayout. This is a breaking change for other consumers that import this helper (e.g., front/src/components/pipelines/results/ResultsGraphs.jsx still calls layoutMaking(selectedChart, graphsToView) and destructures pieLayout), which will cause runtime errors (theme is undefined) and missing layout fields. Either update all call sites in the repo as part of this PR, or keep backward compatibility (e.g., make theme optional with safe defaults and/or continue returning pieLayout when needed).
| function graphsMaking(graphsToView, run, metrics, values, runIndex, theme) { | ||
| graphsToView.radar = graphsToView.radar || []; | ||
| graphsToView.bar = graphsToView.bar || []; | ||
| graphsToView.pie = graphsToView.pie || []; | ||
|
|
||
| const radarTheta = showCustomMetrics ? selectedParameters : generalParameters; | ||
| const radarR = relevantNumericValues; | ||
| const colors = getTraceColors(theme); | ||
| const color = colors[runIndex % colors.length]; | ||
| const runLabel = run.run_name || run.name || `Run ${runIndex + 1}`; |
There was a problem hiding this comment.
graphsMaking now expects (graphsToView, run, metrics, values, runIndex, theme) and no longer supports the previous signature. There are existing callers (e.g., front/src/components/pipelines/results/ResultsGraphs.jsx) still passing the old argument list, which will break at runtime when theme is undefined. Please update those call sites in this PR or make theme optional and preserve the old signature via an adapter.
This pull request refactors the experiment results visualization components to provide a more intuitive and flexible metric selection/filtering experience for users. The changes simplify the UI, replace the previous metric selection logic with a sidebar of checkboxes, and improve the handling of metric splits and chart data. The most important changes are grouped below by theme:
Metric selection and filtering improvements:
ResultsGraphsParameters.jsx, allowing users to select, clear, or select all metrics for chart display.ResultsGraphs.jsxto manage metric selection state, auto-select available splits, and synchronize selected metrics with the chosen split. The component now supports both controlled and uncontrolled split selection and provides callbacks for metric selection actions.UI and component structure changes:
ResultsGraphsLayout.jsxto remove legacy parameter selection and switch components, and to integrate the new metric filter sidebar and chart area. Prop types were updated to reflect the new interface. [1] [2]ResultsGraphsPlot.jsxto show a user-friendly message when no metrics are available for the selected view, and simplified the chart rendering logic.Metric split handling and integration:
SessionVisualization.jsxso that the metric split selector controls both the table and graph views, and passes the selected split and change handler toResultsGraphs. [1] [2] [3]