Add Custom Plots Section #3342

julieg18 · 2023-02-23T22:45:04Z

adds a new section to plots called "Custom" that allows the user to add plots to compare a chosen metric and param.

Demo

~~https://user-images.githubusercontent.com/43496356/221732462-d4ad0eec-9cb7-4e5e-95fb-96268be7ec79.mov~~

Screen.Recording.2023-02-28.at.3.18.46.PM.mov

codeclimate · 2023-02-24T16:29:54Z

webview/src/plots/components/customPlots/customPlotsSlice.ts

+} from 'dvc/src/plots/webview/contract'
+import { addPlotsWithSnapshots, removePlots } from '../plotDataStore'
+
+export interface CustomPlotsState extends Omit<CustomPlotsData, 'plots'> {


Similar blocks of code found in 2 locations. Consider refactoring.

extension/src/plots/model/quickPick.ts

codeclimate · 2023-02-24T16:29:55Z

webview/src/plots/components/customPlots/customPlotsSlice.ts

+  size: DEFAULT_SECTION_SIZES[Section.CUSTOM_PLOTS]
+}
+
+export const customPlotsSlice = createSlice({


Similar blocks of code found in 2 locations. Consider refactoring.

codeclimate · 2023-02-24T16:29:55Z

webview/src/plots/components/customPlots/CustomPlots.tsx

+  }
+
+  return items.length > 0 ? (
+    <div


Similar blocks of code found in 2 locations. Consider refactoring.

codeclimate · 2023-02-24T16:29:55Z

webview/src/plots/components/customPlots/CustomPlot.tsx

+    dispatch(changeDisabledDragIds(enabled ? [] : [id]))
+  }
+
+  return (


Similar blocks of code found in 2 locations. Consider refactoring.

Right now, the CustomPlots and CheckpointPlot components are very similar. I figured we could lower code duplication in a followup when I move the trends plots inside of Custom.

Remember to update the docs/walkthrough when you do that:

extension/src/plots/model/quickPick.ts

webview/src/plots/components/customPlots/CustomPlotsWrapper.tsx

webview/src/plots/components/customPlots/util.ts

julieg18 · 2023-02-28T00:30:09Z

webview/src/plots/components/App.test.tsx

@@ -350,7 +352,10 @@ describe('App', () => {
    expect(screen.getByText('Trends')).toBeInTheDocument()
    expect(screen.getByText('Data Series')).toBeInTheDocument()
    expect(screen.getByText('Images')).toBeInTheDocument()
-    expect(screen.getByText('No Plots to Display')).toBeInTheDocument()
+    expect(screen.getByText('Custom')).toBeInTheDocument()
+    const noPlotsToDisplayElements = screen.getAllByText('No Plots to Display')


Since we now have three sections that show the "No plots to Display" text, I had to update the tests accordingly. Is there a better way to check for the array elements being in the document?

expect the result to have length 2?

expect the result to have length 2?

Now that I look at the documentation, I see that we don't need to do the actual check for toBeInTheDocument. Length would indeed be enough, thanks!

We'd probably still need to map them to their respective section though. Something like within(screen.getByTestId('custom-section')).getByText('No Plots to Display').

julieg18 · 2023-02-28T00:34:41Z

webview/src/plots/components/PlotsContainer.tsx

+    })
+  }
+
+  if (removePlotsButton) {


For now, I've placed the custom plot actions in menu items but these are rather small and possibly could be missed 🤔 Is there some better options to place these actions?

I think it's good for the "add" button. For the "remove" one, I feel like it would be better if it were associated to the plot itself. We do not have anything for this yet, though. We can keep this as is for now until we find something to add actions to a plot.

webview/src/plots/components/PlotsContainer.tsx

julieg18 · 2023-02-28T00:39:48Z

webview/src/plots/components/customPlots/CustomPlot.tsx

+    dispatch(changeDisabledDragIds(enabled ? [] : [id]))
+  }
+
+  return (


Right now, the CustomPlots and CheckpointPlot components are very similar. I figured we could lower code duplication in a followup when I move the trends plots inside of Custom.

webview/src/plots/store.ts

webview/src/plots/components/App.test.tsx

shcheklein · 2023-02-28T02:20:43Z

Thanks @julieg18 ! Looks like a great progress.

Q: to double check. On the plot It seems not all experiments are present (I see 3 dots while there are 4 experiments in the side panel). Is it expected?

webview/src/plots/components/customPlots/util.ts

mattseddon · 2023-02-28T02:55:11Z

extension/src/plots/model/index.ts

+    )
+
+    if (plotAlreadyExists) {
+      return Toast.showError('Custom plot already exists.')


[I] Ideally this class is responsible for data/data manipulation. Plots should use Toast and inform the user when there is a problem.

Maybe we could remove the choice from the list instead of letting them select something that already exists.

Maybe we could remove the choice from the list instead of letting them select something that already exists.

Definitely doable by looping over the custom plots and checking for what exists already!

mattseddon

I did a first pass. Please take a look at the bug here.

After removing trends/de-duplicating and cleaning all of the code we will need to think about the UX of this feature. To me, this emphasises the fragmented nature of experiments + plots. E.g to use I would need to do this:

Screen.Recording.2023-02-28.at.2.03.33.pm.mov

There might not be much that we can do but we should at least consider how we can add something into the experiments table to shortcut the process.

julieg18 · 2023-02-28T13:50:48Z

Q: to double check. On the plot It seems not all experiments are present (I see 3 dots while there are 4 experiments in the side panel). Is it expected?

Yes, it's expected. Two experiments have the exact same params/metrics so they overlap each other, causing the plot to show only three dots.

sroy3

Screen.Recording.2023-02-28.at.10.10.12.AM.mov

The order is not persisted like other sections

sroy3 · 2023-02-28T14:40:03Z

extension/src/plots/model/index.ts

+    )
+
+    if (plotAlreadyExists) {
+      return Toast.showError('Custom plot already exists.')


Maybe we could remove the choice from the list instead of letting them select something that already exists.

extension/src/plots/webview/messages.ts

webview/src/plots/components/App.test.tsx

sroy3 · 2023-02-28T14:49:47Z

webview/src/plots/components/App.test.tsx

@@ -350,7 +352,10 @@ describe('App', () => {
    expect(screen.getByText('Trends')).toBeInTheDocument()
    expect(screen.getByText('Data Series')).toBeInTheDocument()
    expect(screen.getByText('Images')).toBeInTheDocument()
-    expect(screen.getByText('No Plots to Display')).toBeInTheDocument()
+    expect(screen.getByText('Custom')).toBeInTheDocument()
+    const noPlotsToDisplayElements = screen.getAllByText('No Plots to Display')


We'd probably still need to map them to their respective section though. Something like within(screen.getByTestId('custom-section')).getByText('No Plots to Display').

sroy3 · 2023-02-28T14:54:31Z

webview/src/plots/components/PlotsContainer.tsx

+    })
+  }
+
+  if (removePlotsButton) {


I think it's good for the "add" button. For the "remove" one, I feel like it would be better if it were associated to the plot itself. We do not have anything for this yet, though. We can keep this as is for now until we find something to add actions to a plot.

webview/src/plots/components/PlotsContainer.tsx

webview/src/plots/components/customPlots/CustomPlotsWrapper.tsx

julieg18 · 2023-02-28T18:40:41Z

The order is not persisted like other sections

Forgot to add the logic for reordering 😅. Thanks for spotting that! Added

codeclimate · 2023-02-28T18:47:28Z

webview/src/plots/components/customPlots/CustomPlots.tsx

+    setPlotsIdsOrder(changeOrderWithDraggedInfo(order, draggedRef))
+  }
+
+  return items.length > 0 ? (


Similar blocks of code found in 2 locations. Consider refactoring.

codeclimate · 2023-02-28T18:47:29Z

webview/src/plots/components/customPlots/CustomPlots.tsx

+    setOrder(pastOrder => performSimpleOrderedUpdate(pastOrder, plotsIds))
+  }, [plotsIds])
+
+  const setPlotsIdsOrder = (order: string[]): void => {


Similar blocks of code found in 2 locations. Consider refactoring.

dberenbaum · 2023-02-28T19:21:27Z

Not sure what to call this kind of plot. When I see Custom, I would expect that it allows me to plot any of the data I use for my other plots, but with some custom configuration.

This is currently a different and pretty specific type of plot. Maybe Metrics or Metric by Parameter?

dberenbaum · 2023-02-28T20:49:50Z

Which experiments are included in this plot? The same ones that are selected for other plots? All experiments for the current commit? Something else?

julieg18 · 2023-02-28T21:08:08Z

Which experiments are included in this plot? The same ones that are selected for other plots? All experiments for the current commit? Something else?

Currently, all experiments shown in the table webview. In other words, all experiments from the past three commits.

julieg18 · 2023-03-01T15:10:59Z

Alright, hopefully, the main issues have been resolved and other comments can be resolved in a followup! Will merge once the tests pass and open a followup issue detailing next steps!

codeclimate · 2023-03-01T15:22:30Z

Code Climate has analyzed commit 74e70a0 and detected 15 issues on this pull request.

Here's the issue category breakdown:

Category	Count
Complexity	3
Duplication	12

The test coverage on the diff in this pull request is 89.2% (85% is the threshold).

This pull request will bring the total coverage in the repository to 95.9% (0.0% change).

View more on Code Climate.

wip on custom plots section

0d98524

julieg18 added the product PR that affects product label Feb 23, 2023

julieg18 self-assigned this Feb 23, 2023

julieg18 added 5 commits February 23, 2023 17:32

delete plot fix

2da1c1e

some refactoring

165113c

Clear rest of personal comments

175e89e

Fix tests

f571dd9

Merge branch 'main' into add-metric-param-plots

66bd1b1

codeclimate bot reviewed Feb 24, 2023

View reviewed changes

julieg18 and others added 6 commits February 27, 2023 08:56

Add "backend" tests

ac8e910

Add rest of backend tests

2a791c4

Add storybook

1ed3e8f

Add front end tests

e276074

Merge branch 'main' into add-metric-param-plots

67096ab

Fix broken CustomPlot

f6b4876

codeclimate bot reviewed Feb 27, 2023

View reviewed changes

extension/src/plots/model/quickPick.ts Outdated Show resolved Hide resolved

webview/src/plots/components/customPlots/CustomPlotsWrapper.tsx Show resolved Hide resolved

webview/src/plots/components/customPlots/util.ts Show resolved Hide resolved

julieg18 added 5 commits February 27, 2023 11:58

Reduce duplicate code

154b736

Add tests for quick pick

5e75dfb

Remove unneeded types

83392e5

Fix typo

a216927

Remove loading logic in CustomPlots

c7b3afe

julieg18 commented Feb 28, 2023

View reviewed changes

Merge branch 'main' into add-metric-param-plots

981f613

julieg18 marked this pull request as ready for review February 28, 2023 02:09

julieg18 requested review from mattseddon and sroy3 as code owners February 28, 2023 02:09

julieg18 requested a review from shcheklein February 28, 2023 02:09

mattseddon reviewed Feb 28, 2023

View reviewed changes

webview/src/plots/components/customPlots/util.ts Show resolved Hide resolved

mattseddon reviewed Feb 28, 2023

View reviewed changes

Improve testing

93a996b

sroy3 reviewed Feb 28, 2023

View reviewed changes

julieg18 added 4 commits February 28, 2023 09:40

Move Toast code out of model

aacb527

Don't pin 0 on y axis

86f9f07

Fix App.test.js error

6036be3

Make custom plots tooltip more clear

d664b87

julieg18 mentioned this pull request Feb 28, 2023

Delete unused code in Plots #3367

Merged

Add missing custom plots reordering logic

9b4c889

codeclimate bot reviewed Feb 28, 2023

View reviewed changes

sroy3 approved these changes Feb 28, 2023

View reviewed changes

julieg18 and others added 4 commits February 28, 2023 16:24

Merge branch 'main' into add-metric-param-plots

3c2739a

Merge branch 'main' into add-metric-param-plots

b96068e

Have plots update on experiment deletion

aa9e592

Undo menu rename

fbf6e2c

Merge branch 'main' into add-metric-param-plots

74e70a0

julieg18 merged commit 484a604 into main Mar 1, 2023

julieg18 deleted the add-metric-param-plots branch March 1, 2023 15:35

julieg18 mentioned this pull request Mar 1, 2023

Improve Custom Plots #3373

Closed

mattseddon mentioned this pull request Mar 14, 2023

Move trends plots inside of "Custom" section #3404

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Custom Plots Section #3342

Add Custom Plots Section #3342

julieg18 commented Feb 23, 2023 •

edited

Loading

codeclimate bot Feb 24, 2023

codeclimate bot Feb 24, 2023

codeclimate bot Feb 24, 2023

codeclimate bot Feb 24, 2023

julieg18 Feb 28, 2023

mattseddon Feb 28, 2023

julieg18 Feb 28, 2023

mattseddon Feb 28, 2023

julieg18 Feb 28, 2023

sroy3 Feb 28, 2023

julieg18 Feb 28, 2023

sroy3 Feb 28, 2023

julieg18 Feb 28, 2023

shcheklein commented Feb 28, 2023

mattseddon Feb 28, 2023

sroy3 Feb 28, 2023

julieg18 Feb 28, 2023

mattseddon left a comment

julieg18 commented Feb 28, 2023

sroy3 left a comment

sroy3 Feb 28, 2023

sroy3 Feb 28, 2023

sroy3 Feb 28, 2023

julieg18 commented Feb 28, 2023

codeclimate bot Feb 28, 2023

codeclimate bot Feb 28, 2023

dberenbaum commented Feb 28, 2023

dberenbaum commented Feb 28, 2023

julieg18 commented Feb 28, 2023

julieg18 commented Mar 1, 2023

codeclimate bot commented Mar 1, 2023

Add Custom Plots Section #3342

Add Custom Plots Section #3342

Conversation

julieg18 commented Feb 23, 2023 • edited Loading

Demo

codeclimate bot Feb 24, 2023

Choose a reason for hiding this comment

codeclimate bot Feb 24, 2023

Choose a reason for hiding this comment

codeclimate bot Feb 24, 2023

Choose a reason for hiding this comment

codeclimate bot Feb 24, 2023

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shcheklein commented Feb 28, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattseddon left a comment

Choose a reason for hiding this comment

julieg18 commented Feb 28, 2023

sroy3 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

julieg18 commented Feb 28, 2023

codeclimate bot Feb 28, 2023

Choose a reason for hiding this comment

codeclimate bot Feb 28, 2023

Choose a reason for hiding this comment

dberenbaum commented Feb 28, 2023

dberenbaum commented Feb 28, 2023

julieg18 commented Feb 28, 2023

julieg18 commented Mar 1, 2023

codeclimate bot commented Mar 1, 2023

julieg18 commented Feb 23, 2023 •

edited

Loading