feat: Add charts to Comparison View (ET-99) #9215

johnkim-det · 2024-04-22T17:01:40Z

Ticket

ET-99

Description

Adding "Scatter Plots" and "Heat Map" charts to comparison view on Experiments list page

Test Plan

Select a multi-trial experiment to display in comparison view.
Compare what's displayed in the Scatter Plots and Heat Map charts with what's displayed for the experiment's best trial on the Experiment Details page (Visualization tab, HP Scatter Plots and HP Heat Map sub-tabs)
Make sure that the filters for selecting HPs, Metric, and Scale update the charts as expected

Checklist

Changes have been manually QA'd
User-facing API changes need the "User-facing API Change" label.
Release notes should be added as a separate file under docs/release-notes/.
See Release Note for details.
Licenses should be included for new code which was copied and/or modified from any external code.

netlify · 2024-04-22T17:01:57Z

✅ Deploy Preview for determined-ui ready!

Name	Link
🔨 Latest commit	`27cca86`
🔍 Latest deploy log	https://app.netlify.com/sites/determined-ui/deploys/664ca41bc7b123000809021b
😎 Deploy Preview	https://deploy-preview-9215--determined-ui.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

codecov · 2024-04-23T17:56:37Z

Codecov Report

Attention: Patch coverage is 85.72469% with 196 lines in your changes are missing coverage. Please review.

Project coverage is 40.78%. Comparing base (53edec9) to head (27cca86).
Report is 18 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #9215      +/-   ##
==========================================
- Coverage   45.28%   40.78%   -4.51%     
==========================================
  Files        1227      907     -320     
  Lines      154095   115425   -38670     
  Branches     2403     2672     +269     
==========================================
- Hits        69782    47074   -22708     
+ Misses      84121    68174   -15947     
+ Partials      192      177      -15

Flag	Coverage Δ
harness	`?`
web	`40.02% <85.72%> (+3.71%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files	Coverage Δ
.../src/components/CompareHyperparameters.settings.ts	`100.00% <100.00%> (ø)`
...rc/components/CompareHyperparameters.test.mock.tsx	`100.00% <100.00%> (ø)`
...eact/src/components/CompareParallelCoordinates.tsx	`90.86% <100.00%> (+90.86%)`	⬆️
...bui/react/src/components/GalleryModalComponent.tsx	`91.48% <100.00%> (+91.48%)`	⬆️
webui/react/src/components/ParallelCoordinates.tsx	`90.51% <100.00%> (+90.51%)`	⬆️
webui/react/src/types.ts	`99.68% <100.00%> (+<0.01%)`	⬆️
webui/react/src/services/decoder.ts	`20.44% <0.00%> (-0.03%)`	⬇️
webui/react/src/components/ComparisonView.tsx	`0.00% <0.00%> (ø)`
...UPlot/UPlotScatter/useScatterPointTooltipPlugin.ts	`46.38% <33.33%> (+46.38%)`	⬆️
webui/react/src/components/UPlot/UPlotChart.tsx	`61.93% <16.66%> (+48.86%)`	⬆️
... and 6 more

... and 337 files with indirect coverage changes

hkang1

This looks mostly good, haven't had time to try it out but the code looks good. Let some questions and comments through out.

webui/react/src/components/CompareParallelCoordinates.tsx

webui/react/src/components/CompareScatterPlots.tsx

johnkim-det · 2024-04-25T19:09:42Z

Will look at @hkang1's comments but he also let me know that he might not have time for more detailed review. originally asked him because he worked on this before, but now tagging @keita-determined and @thiagodallacqua-hpe too, can you please take a look as well?

keita-determined

not fully reviewed yet, but so far i found these.

only the Heat Map chart goes out of container. is it expected?
qq: iirc, each chart has a tab. All the charts are shown in the single page?

Screen.Recording.2024-04-25.at.2.49.32.PM.mov

theres scroll bar on hover. it looks like a bug

Screen.Recording.2024-04-25.at.3.00.28.PM.mov

keita-determined · 2024-04-25T22:05:42Z

webui/react/src/components/CompareHeatMaps.tsx

+              chartData?.hpMetrics[key] || [],
+              chartData?.trialIds || [],
+            ],
+          ] as FacetedData,


i think we should avoid as as much as possible cuz it leads to the bug easily

discussed with caleb and I think it makes sense to keep this particular assertion for now

nvm, did remove

webui/react/src/components/CompareScatterPlots.tsx

johnkim-det · 2024-04-29T17:05:04Z

updated

fix for horizontal space issue
rename ExperimentHyperparametersSettings
remove type assertions from ScatterPlots

hkang1

The UI looks great. On the details it looks like the fact that heat maps is unable to render categorical axes is a known issue that also exists in the experiment detail so it is not a newly introduced problem. It can probably just be a entirely separate PR to get that resolved. I haven't had a chance to take a real in-depth look but scanning through the code, nothing other than nits stood out, could probably use someone else's more in-depth look to shake out any potential problems.

keita-determined

mostly working good. some finding. also the code is massive so i might add some more comments later

when i select paused experiments, parallel coord is not displayed.

Screen.Recording.2024-05-06.at.3.10.21.PM.mov

webui/react/src/components/CompareHeatMaps.tsx

keita-determined · 2024-05-06T21:41:29Z

webui/react/src/components/CompareScatterPlots.tsx

qq: there is only one axis value in y axis with log scale. is it expected? looks like some axis values are missing.

I think this is okay? I would definitely be concerned if log scale always only displayed one value, but it seems to depend on the value, and I think it makes sense that log scale would not have evenly spaced values.

hmm interesting. yeah ok its not a blocking issue then

keita-determined · 2024-05-06T21:53:32Z

webui/react/src/components/CompareHeatMaps.tsx

log scale isnt applied in heat map

Screen.Recording.2024-05-06.at.2.52.06.PM.mov

nice catch, this was not working on the existing HpHeatMaps chart either

do you think its a blocker or non-blocker?

webui/react/src/components/CompareHeatMaps.tsx

webui/react/src/components/CompareScatterPlots.tsx

johnkim-det · 2024-05-10T20:07:21Z

Pushed fixes for @keita-determined's last review, and also was able to address what @hkang1 mentioned regarding categorical axis labeling.

The one remaining issue right now is what happens with Parallel Coordinates when experiments with different metrics/hyperparameters are compared (I think this is the cause rather than the "Pause" state). FWIW it is a pre-existing issue and arguably could be considered out of scope of this PR, but I'll spend some time looking at it -- if I really can't make progress on it I might push for a separate ticket to address.

keita-determined · 2024-05-10T20:56:38Z

@johnkim-det okay. i'll review the PR after fixing or file the issue.

johnkim-det · 2024-05-13T15:14:50Z

created https://hpe-aiatscale.atlassian.net/browse/ET-261 for the Parallel Coordinates issue.

hkang1

the basic sanity tests around making sure they render and have the titles look good. One suggestion to handle the case if the shape of the mocked data changes (due to API client changing)

webui/react/src/components/CompareHyperparameters.tsx

hkang1 · 2024-05-13T17:43:50Z

webui/react/src/components/CompareHyperparameters.test.mock.tsx

+import { Scale } from 'types';
+
+import CompareHyperparameters from './CompareHyperparameters';
+export const METRIC_DATA = {


for the mock data, are there api-ts-sdk client types that can be associated with it? For example if there was a MetricData type exported from api-ts-sdk then you can do export const METRIC_DATA: MetricData = { and if the shape changes in the future this test would capture the change.

If MetricData is not available and METRIC_DATA is actually a form of a processed data, it might be better to mock the unprocessed data with the corresponding typing and then converting it to METRIC_DATA to capture the benefit mentioned above.

not from api-ts-sdk but adding typing with TrialMetricData from useTrialMetrics

keita-determined

the selected experiments are deselected in a certain conditions. not exactly sure how to reproduce it, but if i select complete experiments and running experiments` it happens. if this bug is not related to the PR, its not a blocker.

Screen.Recording.2024-05-14.at.11.02.28.AM.mov

nit design bug: still see the height change on hover, when there are few charts in the board.

Screen.Recording.2024-05-14.at.11.02.48.AM.mov

johnkim-det · 2024-05-15T17:00:32Z

@keita-determined not sure if this will make sure it never happens, but added a change to Scatter Plots and Heat Maps charts to avoid width being too narrow for tooltip and creating a horizontal scrollbar.

As for the other issue, I think I'm seeing the same thing on latest-main so it's not part of this PR, but I created https://hpe-aiatscale.atlassian.net/browse/ET-264. Can you confirm it's the same thing you're seeing? I'm trying to add more info for reproduction or to try to identify the cause, would be nice if you could look at that too.

keita-determined

one comment, otherwise LGTM
the code around charts has been complicated. hope we can have time to refactor code.

keita-determined · 2024-05-20T22:01:04Z

webui/react/src/components/CompareHyperparameters.tsx

+    resetSettings,
+  } = useSettings<CompareHyperparametersSettings>(settingsConfig);
+
+  const selectedScale = settings.scale;


ideally we wanna remove the useEffect and use the update function in somewhere where settings.scale is updated.
if its hard to do, can we move it into useEffect? looks like its only used in useEffect

keita-determined · 2024-05-20T22:15:44Z

As for the other issue, I think I'm seeing the same thing on latest-main so it's not part of this PR, but I created https://hpe-aiatscale.atlassian.net/browse/ET-264. Can you confirm it's the same thing you're seeing? I'm trying to add more info for reproduction or to try to identify the cause, would be nice if you could look at that too.

looks like its the same issue

johnkim-det added 6 commits April 19, 2024 11:58

CompareScatterPlots

d7e4677

UPlotChart fix

73202d8

CompareHeatMaps

ea1036a

UPlotChart fix

30b6f7b

fmt

5f6f04b

typo

76da5ca

johnkim-det requested a review from a team as a code owner April 22, 2024 17:01

johnkim-det requested a review from thiagodallacqua-hpe April 22, 2024 17:01

cla-bot bot added the cla-signed label Apr 22, 2024

Merge branch 'main' into ET-99

232a4b0

johnkim-det requested review from hkang1 and removed request for thiagodallacqua-hpe April 22, 2024 17:02

johnkim-det added 2 commits April 23, 2024 13:28

Merge branch 'main' into ET-99

60a7816

include config in BulkExperimentItem

6660ce0

johnkim-det assigned hkang1 Apr 23, 2024

hkang1 reviewed Apr 23, 2024

View reviewed changes

johnkim-det added 2 commits April 23, 2024 14:04

include config in decoder

8a7a76a

improve smallerIsBetter check

bc37a72

johnkim-det requested review from keita-determined and thiagodallacqua-hpe April 25, 2024 19:05

keita-determined reviewed Apr 25, 2024

View reviewed changes

johnkim-det added 2 commits April 29, 2024 11:59

fixes

5ded304

fmt

68e0bf2

johnkim-det requested review from hkang1 and keita-determined April 29, 2024 17:00

add string to UPlotData type

32e4129

hkang1 assigned johnkim-det and unassigned hkang1 May 2, 2024

johnkim-det added 2 commits May 2, 2024 11:22

remove type assertions from Hp charts also

2e25646

loading state and tests

c9e59f3

hkang1 approved these changes May 6, 2024

View reviewed changes

keita-determined reviewed May 6, 2024

View reviewed changes

johnkim-det added 3 commits May 10, 2024 11:10

Merge branch 'main' into ET-99

a06646c

fixes

0d671cf

fix categorical axes labeling

5c85f6b

johnkim-det requested review from keita-determined and hkang1 May 13, 2024 15:14

hkang1 approved these changes May 13, 2024

View reviewed changes

johnkim-det added 2 commits May 14, 2024 10:51

fixes

e96295f

fix

fa2c413

keita-determined reviewed May 14, 2024

View reviewed changes

johnkim-det added 2 commits May 15, 2024 12:34

increase grid item widths

86ca1b4

Merge branch 'main' into ET-99

b1eb36e

Merge branch 'main' into ET-99

eaa8d95

johnkim-det requested a review from keita-determined May 15, 2024 20:35

johnkim-det assigned keita-determined and unassigned johnkim-det May 20, 2024

keita-determined approved these changes May 20, 2024

View reviewed changes

setScale

27cca86

johnkim-det merged commit cb81a44 into main May 21, 2024
83 of 96 checks passed

johnkim-det deleted the ET-99 branch May 21, 2024 15:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add charts to Comparison View (ET-99) #9215

feat: Add charts to Comparison View (ET-99) #9215

johnkim-det commented Apr 22, 2024 •

edited by jira bot

Loading

netlify bot commented Apr 22, 2024 •

edited

Loading

codecov bot commented Apr 23, 2024 •

edited

Loading

hkang1 left a comment

johnkim-det commented Apr 25, 2024

keita-determined left a comment

keita-determined Apr 25, 2024

johnkim-det Apr 29, 2024

johnkim-det May 6, 2024

johnkim-det commented Apr 29, 2024

hkang1 left a comment

keita-determined left a comment

keita-determined May 6, 2024

johnkim-det May 10, 2024

keita-determined May 10, 2024

keita-determined May 6, 2024

johnkim-det May 10, 2024

keita-determined May 10, 2024

johnkim-det commented May 10, 2024

keita-determined commented May 10, 2024

johnkim-det commented May 13, 2024

hkang1 left a comment

hkang1 May 13, 2024

johnkim-det May 14, 2024

keita-determined left a comment

johnkim-det commented May 15, 2024

keita-determined left a comment

keita-determined May 20, 2024 •

edited

Loading

keita-determined commented May 20, 2024

feat: Add charts to Comparison View (ET-99) #9215

feat: Add charts to Comparison View (ET-99) #9215

Conversation

johnkim-det commented Apr 22, 2024 • edited by jira bot Loading

Ticket

Description

Test Plan

Checklist

netlify bot commented Apr 22, 2024 • edited Loading

✅ Deploy Preview for determined-ui ready!

codecov bot commented Apr 23, 2024 • edited Loading

Codecov Report

hkang1 left a comment

Choose a reason for hiding this comment

johnkim-det commented Apr 25, 2024

keita-determined left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johnkim-det commented Apr 29, 2024

hkang1 left a comment

Choose a reason for hiding this comment

keita-determined left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johnkim-det commented May 10, 2024

keita-determined commented May 10, 2024

johnkim-det commented May 13, 2024

hkang1 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

keita-determined left a comment

Choose a reason for hiding this comment

johnkim-det commented May 15, 2024

keita-determined left a comment

Choose a reason for hiding this comment

keita-determined May 20, 2024 • edited Loading

Choose a reason for hiding this comment

keita-determined commented May 20, 2024

johnkim-det commented Apr 22, 2024 •

edited by jira bot

Loading

netlify bot commented Apr 22, 2024 •

edited

Loading

codecov bot commented Apr 23, 2024 •

edited

Loading

keita-determined May 20, 2024 •

edited

Loading