Skip to content

Conversation

@mattseddon
Copy link
Contributor

@mattseddon mattseddon commented Sep 23, 2022

2/2 main <- #2403 <- this

Related to #1757

This PR adds shape and stroke dash information into the plots tree.

Screenshots

Screen Shot 2022-09-23 at 4 07 41 pm

image

Note: The hacky way that we add shape + detail to the plot template causes the on-hover behaviour to break. I will change the approach in the next PR.

image

@mattseddon mattseddon added the product PR that affects product label Sep 23, 2022
@mattseddon mattseddon self-assigned this Sep 23, 2022
@mattseddon mattseddon changed the base branch from main to accomodate-flexible-plots September 23, 2022 06:17
@mattseddon
Copy link
Contributor Author

In order to test set MIN_CLI_VERSION = '2.0.0' and use the same repository from #2403. In order to get shapes, I updated the script to the following:

#!/bin/bash

set -exu

wsp=test_wspace_1
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 "some_param: 0.1" > params.yaml


echo "
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, 'y': i*p*random.random()})

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 "plots:
  res2.json:
    template: simple
  combined:
    x: x
    y:
      res1.json: y
      res2.json: [y,z]
  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 "some_param: 0.3
" > params.yaml

dvc exp run -n "p0.9"

echo "some_param: 0.9
" > params.yaml

dvc repro

@mattseddon mattseddon changed the base branch from accomodate-flexible-plots to main September 27, 2022 00:54
@mattseddon mattseddon changed the base branch from main to accomodate-flexible-plots September 27, 2022 02:29
@mattseddon mattseddon changed the base branch from accomodate-flexible-plots to main September 27, 2022 05:16
@mattseddon mattseddon changed the base branch from main to accomodate-flexible-plots September 27, 2022 05:18
@mattseddon mattseddon marked this pull request as ready for review September 27, 2022 05:20
legend: {
disable: true,
symbolFillColor: 'transparent',
symbolStrokeColor: 'grey'
Copy link
Contributor

Choose a reason for hiding this comment

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

Should these be set where it's enabled only? (Line 311)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am going to have to change the approach slightly now that we don't know exactly where the encoding update has been made.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will call out the reasoning for leaving in the follow-up.

legend: {
disable: false
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

What changed here? Is that only a lint/prettier config?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, I can revert

Copy link
Contributor

Choose a reason for hiding this comment

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

That's fine, I was just wondering if there is something I was missing. Better keep it that way IMO

Base automatically changed from accomodate-flexible-plots to main September 28, 2022 02:29
@mattseddon mattseddon enabled auto-merge (squash) September 28, 2022 02:36
@qlty-cloud-legacy
Copy link

Code Climate has analyzed commit e10c8d6 and detected 2 issues on this pull request.

Here's the issue category breakdown:

Category Count
Duplication 2

The test coverage on the diff in this pull request is 94.9% (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.

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.

2 participants