Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Render flexible plots #2403

Merged
merged 19 commits into from
Sep 28, 2022
Merged

Render flexible plots #2403

merged 19 commits into from
Sep 28, 2022

Conversation

mattseddon
Copy link
Member

@mattseddon mattseddon commented Sep 15, 2022

1/2 main <- this <- #2452

Related to #1757

This PR is the first major step in accommodating the data required to render flexible plots.

I have updated our approach to check each datapoint in the plots data for an object named dvc_data_version_info. This object can currently contain a field or filename. Multi-source (flexible) plots are identified by > 1 variation in this data.

After collecting all of the variations in the data we then work out if we need to and how to apply a different strokeDash/shape for each variation.

These generate an encoding update which is passed to the webview as part of the template.

image

Screen Shot 2022-09-15 at 3 13 04 pm

Note: The next step is to add the new items shown in the zoomed-in plot's legend into the plots tree in the sidebar.

In order to test manually you would need a repo with flexible plots and this branch: iterative/dvc#8282

@mattseddon mattseddon added the product PR that affects product label Sep 15, 2022
@mattseddon mattseddon self-assigned this Sep 15, 2022
pared
pared previously requested changes Sep 15, 2022
extension/src/plots/model/collect.ts Outdated Show resolved Hide resolved
@mattseddon mattseddon force-pushed the accomodate-flexible-plots branch from 1519e4d to 2590045 Compare September 19, 2022 09:42
extension/src/plots/model/collect.ts Outdated Show resolved Hide resolved
extension/src/plots/model/collect.ts Outdated Show resolved Hide resolved
extension/src/plots/model/collect.ts Outdated Show resolved Hide resolved
extension/src/plots/model/collect.ts Outdated Show resolved Hide resolved
extension/src/plots/model/collect.ts Outdated Show resolved Hide resolved
@mattseddon mattseddon force-pushed the accomodate-flexible-plots branch from f07fb7d to d5e515d Compare September 20, 2022 03:06
extension/src/plots/model/collect.ts Outdated Show resolved Hide resolved
extension/src/plots/model/collect.ts Outdated Show resolved Hide resolved
[4, 4],
[4, 2],
[2, 1],
[1, 1]
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

export type StrokeDashValue = typeof StrokeDash[number]

export const Shape = ['square', 'circle', 'triangle'] as const
export type ShapeValue = typeof Shape[number]
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.


describe('reverseOfLegendSuppressionUpdate', () => {
it('should reverse the legend suppression applied by extendVegaSpec', () => {
type NonOptionalEncoding = { [P in keyof Encoding]-?: Encoding[P] }
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[F] This means that if the Encoding property is updated then compilation will fail unless this test is also updated.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Which should mean we don't miss the required webview update.

color: {
domain: expectedRevisions,
range: copyOriginalColors().slice(0, 5)
}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[F] We will need integration tests later.

const obj = data as Record<string, unknown>
return {
...obj,
[field]: mergeFields(fields.map(field => obj[field] as string))
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[F] There is a situation where we concatenate fields together for the stoke dash update. If that happens we need to do the same thing with the data.

@mattseddon mattseddon marked this pull request as ready for review September 23, 2022 02:33
@mattseddon mattseddon changed the title Accomodate flexible plots Render flexible plots Sep 23, 2022
@julieg18
Copy link
Contributor

Excited to test this out!

In order to test manually you would need a repo with flexible plots and this branch: iterative/dvc#8282

I'm not very familiar with DVC plots. Does anyone know a repo with flexible plots? Or is there a way to alter a DVC repo to product flexible plots?

@pared
Copy link

pared commented Sep 23, 2022

@julieg18
I usually create them via some script:

#!/bin/bash

set -exu

wsp=test_wspace
rep=test_repo

rm -rf $wsp && mkdir $wsp && pushd $wsp
main=$(pwd)

mkdir $rep && pushd $rep

git init
dvc init

mkdir subdir && pushd subdir

git add -A
git commit -am "initial"

echo -e "some_param: 0.1
" > params.yaml


echo -e "
import yaml
import json
import random

def get_params():
    with open('params.yaml', 'r') as fd:
        return yaml.safe_load(fd)
p = get_params()['some_param']


def prepare_cm(num_classes=3,num_samples=3):
    result = []
    for i in range(num_classes):
        for j in range(num_samples):
            if random.random() <= p:
                result.append({'actual': i, 'predicted': i})
            else:
                result.append({'actual': i, 'predicted': random.choice(list(range(num_samples)))})
    return result

res1=[]
res2=[]
for i in range(1, 11):
    res1.append({'x': i, 'y': i*p})
    res2.append({'x': i, 'z': i*p*2})

with open('res1.json', 'w') as fd:
    json.dump(res1, fd)

with open('res2.json', 'w') as fd:
    json.dump(res2, fd)

with open('metric1.json', 'w') as fd:
    json.dump(res1[-1],fd)

with open('metric2.json', 'w') as fd:
    json.dump(res2[-1],fd)

with open('confusion_matrix.json', 'w') as fd:
    json.dump(prepare_cm(), fd)

import shutil
shutil.copyfile('confusion_matrix.json', 'confusion_matrix2.json')
" > train.py

dvc run -d train.py -p some_param \
--plots-no-cache confusion_matrix.json \
--plots-no-cache res1.json \
-O res2.json -M metric1.json -M metric2.json \
-O confusion_matrix2.json -n train "python train.py"

echo -e "plots:
  res2.json:
    template: simple
  combined:
    x: x
    y:
      res1.json: y
      res2.json: y
  confusion_matrix2.json:
    template: confusion
    x: actual
    y: predicted" >> dvc.yaml
 

dvc plots modify res1.json --template scatter -x x -y y
dvc plots modify confusion_matrix.json --template confusion -x actual -y predicted

git add -A
git commit -am "initial run"

echo -e "some_param: 0.3
" > params.yaml

dvc exp run -n "p0.9"

echo -e "some_param: 0.9
" > params.yaml

dvc repro

Flexible plots are the ones that are created via top level plot definition in dvc.yaml (in given script its the code appending plots section to dvc.yaml).

@sroy3
Copy link
Contributor

sroy3 commented Sep 26, 2022

A little CSS bug:

Screen.Recording.2022-09-26.at.9.48.09.AM.mov

Same thing happening in a non-zoomed-in-plot:

Screen.Recording.2022-09-26.at.9.49.44.AM.mov

@mattseddon
Copy link
Member Author

A little CSS bug:

Screen.Recording.2022-09-26.at.9.48.09.AM.mov
Same thing happening in a non-zoomed-in-plot:

Screen.Recording.2022-09-26.at.9.49.44.AM.mov

Is this new or existing?

@sroy3
Copy link
Contributor

sroy3 commented Sep 26, 2022

A little CSS bug:
Screen.Recording.2022-09-26.at.9.48.09.AM.mov
Same thing happening in a non-zoomed-in-plot:
Screen.Recording.2022-09-26.at.9.49.44.AM.mov

Is this new or existing?

Not sure, can only reproduce with the flexible plot. That is because the tooltip goes above the plot. I couldn't find any other plot that has a point that high.

@julieg18
Copy link
Contributor

A little CSS bug:

Is this new or existing?

Appears to be existing since I can replicate in an example-get-started plot:

Screen.Recording.2022-09-26.at.4.56.21.PM.mov

@mattseddon
Copy link
Member Author

Will follow up on the css bug separately.

@mattseddon mattseddon enabled auto-merge (squash) September 27, 2022 23:36
@codeclimate
Copy link

codeclimate bot commented Sep 27, 2022

Code Climate has analyzed commit 36e8013 and detected 1 issue on this pull request.

Here's the issue category breakdown:

Category Count
Complexity 1

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

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

View more on Code Climate.

@mattseddon mattseddon merged commit a6ca465 into main Sep 28, 2022
@mattseddon mattseddon deleted the accomodate-flexible-plots branch September 28, 2022 02:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
product PR that affects product
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants