Add "Image by Step" plots #4372

julieg18 · 2023-07-27T22:22:04Z

`main` <= #4319 <= this

Demo

~~https://github.com/iterative/vscode-dvc/assets/43496356/e84c4fd1-a6fd-43e4-95a3-b5bbc3b5b649~~

~~https://github.com/iterative/vscode-dvc/assets/43496356/b49f1401-c955-4507-ba8a-c25e99be5d78~~

Screen.Recording.2023-08-02.at.10.58.39.AM.mov

This reverts commit 739d752.

webview/src/plots/components/comparisonTable/ComparisonTableRow.tsx

…-plots

julieg18 · 2023-07-30T20:28:44Z

extension/src/test/fixtures/plotsDiff/index.ts

-    const multiImagePath = joinFunc('plots', 'image')
-    const isMulti = path.includes(multiImagePath)
-    const pathLabel = path
+    const pathLabel = path.includes('image') ? join('plots', 'image') : path


We had to use join here so the tests would pass on windows (we rebuild the image path when collecting the plots).

extension/src/plots/paths/model.ts

julieg18 · 2023-07-30T20:32:05Z

webview/src/plots/components/comparisonTable/cell/ComparisonTableMultiCell.tsx

+      onClick={() => zoomPlot(url)}
+      data-testid="image-plot-button"
+    >
+      <img


Things that I thought could be done in a followup:

Currently we show the step based off the index of the array which wouldn't work correctly if the user deselected some of the images in the plots sideview tree. A better way to calculate the step could be relying on the image file name or getting the index when collecting the images on the backend.

Save the multiValues across sessions.

Currently we show the step based off the index of the array which wouldn't work correctly if the user deselected some of the images in the plots sideview tree. A better way to calculate the step could be relying on the image file name or getting the index when collecting the images on the backend.

Should we treat the whole thing as a single plot and not allowed selecting/de-selecting of steps?

Save the multiValues across sessions.

Are the plot steps reset when an experiment runs, or is that handled?

Should we treat the whole thing as a single plot and not allowed selecting/de-selecting of steps?

Yes, we could do that! That would make image and image path collection easier! Apologies if that was your original idea in #4319 (comment), I misunderstood 😊

It definitely wasn't the original idea but let's try and get rid of isMulti as a flag if possible.

Adjusted the code to turn off selection/de-selection of steps.

Are the plot steps reset when an experiment runs, or is that handled?

Nope, the plot steps would only reset if you close the "session". They'll keep their current value during an experiment run :)

mattseddon · 2023-07-30T21:20:09Z

@julieg18 I've tried updating train.py in the demo project to log an image per step but it isn't getting picked up by the extension, is this expected? Does there need to be updates made to DVCLive/DVC to get this to work?

julieg18 · 2023-07-30T21:24:38Z

@julieg18 I've tried updating train.py in the demo project to log an image per step but it isn't getting picked up by the extension, is this expected? Does there need to be updates made to DVCLive/DVC to get this to work?

What change did you make?

This was the change I made to get the images:

 # Iterate over training epochs.
    for epoch in range(params["epochs"]):
        # ...rest of code 
        live.log_image(f"image/{epoch}.jpg", missclassified)

mattseddon · 2023-07-30T21:42:48Z

@julieg18 I've tried updating train.py in the demo project to log an image per step but it isn't getting picked up by the extension, is this expected? Does there need to be updates made to DVCLive/DVC to get this to work?

What change did you make?

This was the change I made to get the images:
 # Iterate over training epochs.
    for epoch in range(params["epochs"]):
        # ...rest of code 
        live.log_image(f"image/{epoch}.jpg", missclassified)

Yeah, that is the change I made: live.log_image(f"misclassified/{epoch}.jpg", missclassified)

☝🏻 is image being in the path hard-coded to identify multi-step plots?

julieg18 · 2023-07-30T21:44:56Z

Yeah, that is the change I made: live.log_image(f"misclassified/{epoch}.jpg", misclassified)
☝🏻 is image being in the path hard-coded to identify multi-step plots?

Yes, that is what was decided in iterative/dvclive#425. You'd need to put image as the directory.

mattseddon · 2023-07-30T21:49:24Z

Yeah, that is the change I made: live.log_image(f"misclassified/{epoch}.jpg", misclassified)
☝🏻 is image being in the path hard-coded to identify multi-step plots?

Yes, that is what was decided in iterative/dvclive#425. You'd need to put image as the directory.

Can you please double-check that image was not just a placeholder for the actual image name? What if there are two images that you want to save for each step? Would you have to do:

misclassified/image/{step} & other_image/image/{step}

?

julieg18 · 2023-07-30T22:01:15Z

Can you please double-check that image was not just a placeholder for the actual image name? What if there are two images that you want to save for each step?

Oh, you're right, that's probably what was implied and I took things too literally 🤦‍♀️. I'll adjust the check we use.

…-plots

extension/src/plots/model/collect.ts

julieg18 · 2023-07-31T17:26:21Z

extension/src/plots/model/collect.ts

+  for (const [label, paths] of Object.entries(acc.comparisonData)) {
+    for (const path of Object.keys(paths)) {
+      acc.comparisonData[label][path].sort((img1, img2) =>
+        img1.path.localeCompare(img2.path, undefined, { numeric: true })


dvc plots diff doesn't give you directly images in the step order so we need to sort them on our end.

julieg18 · 2023-07-31T17:32:17Z

webview/src/plots/components/comparisonTable/cell/ComparisonTableMultiCell.tsx

+      onClick={() => zoomPlot(url)}
+      data-testid="image-plot-button"
+    >
+      <img


Adjusted the code to turn off selection/de-selection of steps.

Are the plot steps reset when an experiment runs, or is that handled?

Nope, the plot steps would only reset if you close the "session". They'll keep their current value during an experiment run :)

julieg18 · 2023-07-31T17:35:39Z

webview/src/plots/components/comparisonTable/styles.module.scss

+.multiImageWrapper {
+  .image,
+  .noImageContent {
+    height: 380px;


Had to have a fixed height for multi images since we need all images in the multi plots to be the same height:

Screen.Recording.2023-07-31.at.12.34.10.PM.mov

There should be some ways to at least get an average height based off the images instead of just using a fixed number or apply some CSS that keeps the imgs stable but I figured it could be saved for a followup.

This looks really weird. Definitely need to fix.

This looks really weird. Definitely need to fix.

Understood! I didn't think it looked that bad, but if you think it looks really weird, I'm happy to work on fixing it before merging.

Can still be done in follow-up. Just make sure we don't move on before it is fixed 🙏🏻.

extension/src/cli/dvc/constants.ts

extension/src/plots/model/collect.ts

extension/src/plots/paths/collect.test.ts

extension/src/plots/paths/collect.ts

mattseddon · 2023-07-31T23:22:50Z

webview/src/plots/components/comparisonTable/cell/ComparisonTableMultiCell.tsx

+  if (!stateStep) {
+    return 0
+  }
+  return stateStep > imgsLength - 1 ? imgsLength - 1 : stateStep


[F] This behaviour isn't very intuitive. I had no idea what was happening when running a new experiment. I would expect the image to appear as missing if the image for that step does not exist yet. This will probably simplify the code as well

explanatory variable for imgsLength - 1 would also be good.

This behaviour isn't very intuitive. I had no idea what was happening when running a new experiment. I would expect the image to appear as missing if the image for that step does not exist yet. This will probably simplify the code as well

I chose the new behavior since I was avoiding our sliders having mutatable max length. Example:

Screen.Recording.2023-08-01.at.10.55.03.AM.mov

But if you think the behavior above is preferred though, I'm happy to change it. We also could look into finding the max length across all the plots in the backend so the sliders aren't as buggy. Can we save this change for a followup?

I think the slider should have the max number of available steps for all experiments but also show missing if that step is not available for that experiment.

I think the slider should have the max number of available steps for all experiments but also show missing if that step is not available for that experiment.

Ok, so just making sure I'm understanding things correctly. Let's say we have two experiments, one with 12 images, and one with 5 images. We want the plots to render two plots, each with a slider with a range of 0-11?

Maybe we are overcomplicating and shouldn't be syncing these sliders across experiments. It sounded nice, but it might occasionally be frustrating if the user actually wants to compare different steps. We could always further optimize later. Maybe @daavoo or others have ideas here.

Maybe we are overcomplicating and shouldn't be syncing these sliders across experiments. It sounded nice, but it might occasionally be frustrating if the user actually wants to compare different steps. We could always further optimize later. Maybe @daavoo or others have ideas here.

I am ok with not syncing sliders

Makes sense! I can remove the syncing logic for now and we can try out different ideas in a followup.

Sorry for the flip flopping here @julieg18!

webview/src/plots/components/comparisonTable/cell/ComparisonTableMultiCell.tsx

mattseddon · 2023-07-31T23:52:55Z

webview/src/plots/components/comparisonTable/styles.module.scss

+.multiImageWrapper {
+  .image,
+  .noImageContent {
+    height: 380px;


This looks really weird. Definitely need to fix.

mattseddon · 2023-07-31T23:56:03Z

webview/src/plots/components/comparisonTable/ComparisonTableRows.tsx

-            id: column.id
+            id: column.id,
+            imgs: revs[column.id]?.imgs || [
+              { errors: undefined, loading: false, url: undefined }


[Q] Shouldn't this be taken care of in the backend?

There's a frontend test called should add a new column if there is a new revision that sends data that includes an extra revision but no plot data that matches the revision. I thought that there was a chance of this actually happening so I adjusted the code to handle it. But is the test actually incorrectly written and there is no chance of the backend sending data like that?

extension/src/plots/model/collect.ts

julieg18 · 2023-08-04T15:45:23Z

Merging and taking care of the following comments in a followup:

julieg18 added 3 commits July 27, 2023 17:17

Add back changes in 76ee298

3d8404e

Revert "Simplify first pr changes"

ffd42f8

This reverts commit 739d752.

Delete .gitignore

4af98dd

julieg18 added the product PR that affects product label Jul 27, 2023

julieg18 self-assigned this Jul 27, 2023

julieg18 changed the base branch from main to add-comparison-multi-image-plot July 27, 2023 22:22

julieg18 mentioned this pull request Jul 27, 2023

Add "image by step" plots to comparison section #4319

Merged

fix revert issues

119b63a

mattseddon reviewed Jul 28, 2023

View reviewed changes

webview/src/plots/components/comparisonTable/ComparisonTableRow.tsx Outdated Show resolved Hide resolved

julieg18 added 10 commits July 28, 2023 11:30

Merge branch 'add-comparison-multi-image-plot' into add-image-by-step…

3d825d9

…-plots

Clean up paths model

b6114e8

Fix slider breaking for missing images

00d905b

Fix jest tests

8a60a60

Add test for frontend

6ef31f0

Add missing story

7f82d98

Add tests for paths

1149004

Add vscode tests

39f9caa

Fix typos

32ac3fc

Try to fix vscode tests

a98e068

julieg18 commented Jul 30, 2023

View reviewed changes

julieg18 added 3 commits July 30, 2023 18:03

Merge branch 'add-comparison-multi-image-plot' into add-image-by-step…

feb5cb2

…-plots

Save paths in a directory

a5ded17

Fix possible error being thrown

866df90

julieg18 commented Jul 31, 2023

View reviewed changes

julieg18 marked this pull request as ready for review July 31, 2023 17:36

julieg18 requested a review from sroy3 as a code owner July 31, 2023 17:36

julieg18 marked this pull request as draft July 31, 2023 17:36

Fix error styles

f1723e4

julieg18 marked this pull request as ready for review July 31, 2023 17:39

julieg18 requested review from mattseddon and shcheklein July 31, 2023 17:40

mattseddon approved these changes Aug 1, 2023

View reviewed changes

Refactor improvements

e94143c

dberenbaum mentioned this pull request Aug 2, 2023

Log multiple image plots per step iterative/dvclive#425

Closed

4 tasks

julieg18 added 6 commits August 2, 2023 10:31

Try to improve collectOrderedPath

f594ec1

Remove slider sync

e868630

Fix typo in test

6be47b4

Improve regex and added tests

7c97263

Use multi images in base fixtures

530f8c2

Improve tests

9fb3b7e

julieg18 merged commit 5b13368 into add-comparison-multi-image-plot Aug 4, 2023

julieg18 deleted the add-image-by-step-plots branch August 4, 2023 15:45

mattseddon mentioned this pull request Aug 10, 2023

Fix Comparison Plot "Image By Step" height #4458

Merged

mattseddon mentioned this pull request Sep 12, 2023

plots: standardise across DVC/Studio/VS Code iterative/dvc#9940

Closed

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add "Image by Step" plots #4372

Add "Image by Step" plots #4372

julieg18 commented Jul 27, 2023 •

edited

Loading

julieg18 Jul 30, 2023 •

edited

Loading

julieg18 Jul 30, 2023

mattseddon Jul 30, 2023

julieg18 Jul 30, 2023

mattseddon Jul 31, 2023

julieg18 Jul 31, 2023

mattseddon commented Jul 30, 2023

julieg18 commented Jul 30, 2023

mattseddon commented Jul 30, 2023

julieg18 commented Jul 30, 2023

mattseddon commented Jul 30, 2023

julieg18 commented Jul 30, 2023

julieg18 Jul 31, 2023

julieg18 Jul 31, 2023

julieg18 Jul 31, 2023 •

edited

Loading

mattseddon Jul 31, 2023

julieg18 Aug 1, 2023

mattseddon Aug 1, 2023

mattseddon Jul 31, 2023

mattseddon Jul 31, 2023

julieg18 Aug 1, 2023 •

edited

Loading

mattseddon Aug 1, 2023

julieg18 Aug 2, 2023

dberenbaum Aug 2, 2023

daavoo Aug 2, 2023

julieg18 Aug 2, 2023

dberenbaum Aug 2, 2023

mattseddon Jul 31, 2023

mattseddon Jul 31, 2023

julieg18 Aug 2, 2023 •

edited

Loading

julieg18 commented Aug 4, 2023 •

edited

Loading

Add "Image by Step" plots #4372

Add "Image by Step" plots #4372

Conversation

julieg18 commented Jul 27, 2023 • edited Loading

main <= #4319 <= this

Demo

julieg18 Jul 30, 2023 • edited Loading

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

mattseddon commented Jul 30, 2023

julieg18 commented Jul 30, 2023

mattseddon commented Jul 30, 2023

julieg18 commented Jul 30, 2023

mattseddon commented Jul 30, 2023

julieg18 commented Jul 30, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

julieg18 Jul 31, 2023 • edited Loading

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

julieg18 Aug 1, 2023 • edited Loading

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

julieg18 Aug 2, 2023 • edited Loading

Choose a reason for hiding this comment

julieg18 commented Aug 4, 2023 • edited Loading

julieg18 commented Jul 27, 2023 •

edited

Loading

`main` <= #4319 <= this

julieg18 Jul 30, 2023 •

edited

Loading

julieg18 Jul 31, 2023 •

edited

Loading

julieg18 Aug 1, 2023 •

edited

Loading

julieg18 Aug 2, 2023 •

edited

Loading

julieg18 commented Aug 4, 2023 •

edited

Loading