Skip to content

metrics: 1.0 update#1360

Merged
shcheklein merged 11 commits into
masterfrom
metr-1.0
Jun 1, 2020
Merged

metrics: 1.0 update#1360
shcheklein merged 11 commits into
masterfrom
metr-1.0

Conversation

@dmpetrov
Copy link
Copy Markdown
Contributor

DVC 1.0 metrics

@shcheklein shcheklein temporarily deployed to dvc-landing-metr-1-0-pga0r0rsj May 26, 2020 10:56 Inactive
@calibre-analytics

This comment has been minimized.

@shcheklein shcheklein temporarily deployed to dvc-landing-metr-1-0-pga0r0rsj May 27, 2020 07:46 Inactive
@jorgeorpinel jorgeorpinel changed the title Update metrics metrics: 1.0 update May 27, 2020
Copy link
Copy Markdown
Contributor

@jorgeorpinel jorgeorpinel left a comment

Choose a reason for hiding this comment

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

First review, mostly copy edits on index page. Same feedback applies on similar parts of the other 2 docs.

Comment thread content/docs/command-reference/metrics/diff.md Outdated
Comment thread content/docs/command-reference/metrics/show.md Outdated
Comment thread content/docs/command-reference/metrics/index.md Outdated
Comment thread content/docs/command-reference/metrics/index.md Outdated
Comment thread content/docs/command-reference/metrics/index.md Outdated
Comment thread content/docs/command-reference/metrics/index.md Outdated
Comment thread content/docs/command-reference/metrics/index.md Outdated
Comment thread content/docs/command-reference/metrics/index.md Outdated
Comment thread content/docs/command-reference/metrics/index.md Outdated
Comment thread content/docs/command-reference/metrics/index.md Outdated
Comment thread content/docs/command-reference/metrics/add.md
@jorgeorpinel jorgeorpinel temporarily deployed to dvc-landing-metr-1-0-pga0r0rsj May 29, 2020 07:38 Inactive
jorgeorpinel
jorgeorpinel previously approved these changes May 29, 2020
Copy link
Copy Markdown
Contributor

@jorgeorpinel jorgeorpinel left a comment

Choose a reason for hiding this comment

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

Approving so this can be merged but lots of the changes are mine at this point, some one else may want to do a separate review?

@jorgeorpinel jorgeorpinel requested a review from shcheklein May 29, 2020 07:48
@jorgeorpinel jorgeorpinel temporarily deployed to dvc-landing-metr-1-0-pga0r0rsj May 29, 2020 08:00 Inactive
Comment thread content/docs/command-reference/metrics/show.md Outdated
Comment thread content/docs/command-reference/metrics/diff.md Outdated
Comment thread content/docs/command-reference/metrics/diff.md Outdated
Comment thread content/docs/command-reference/metrics/index.md Outdated
Comment thread content/docs/command-reference/remote/list.md
Comment thread content/docs/command-reference/plots/show.md Outdated
Comment thread content/docs/command-reference/metrics/index.md Outdated
Comment thread content/docs/command-reference/metrics/index.md Outdated
Comment thread content/docs/command-reference/metrics/index.md Outdated
Comment thread content/docs/command-reference/metrics/show.md
Comment thread content/docs/command-reference/metrics/diff.md
Comment thread content/docs/command-reference/metrics/index.md
Comment thread content/docs/command-reference/metrics/diff.md Outdated
Comment thread content/docs/command-reference/metrics/show.md
@jorgeorpinel jorgeorpinel temporarily deployed to dvc-landing-metr-1-0-pga0r0rsj May 30, 2020 20:25 Inactive
@jorgeorpinel jorgeorpinel temporarily deployed to dvc-landing-metr-1-0-pga0r0rsj May 30, 2020 21:45 Inactive
@jorgeorpinel jorgeorpinel requested a review from shcheklein May 30, 2020 21:46
@jorgeorpinel jorgeorpinel dismissed their stale review May 30, 2020 21:48

Ivan is doing a final check.

Comment thread content/docs/command-reference/metrics/diff.md Outdated
DVC itself does not ascribe any specific meaning for these numbers. Usually
these numbers are produced by the model training or model evaluation code and
serve as a way to compare and pick the best performing experiment.
### Types of metrics
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

this should go before description

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I didn't think you meant that literally earlier since it breaks our standard cmd ref structure. But OK, did it now in 3b898b3 (included also continuous and scalar terminology update discussed in Slack).

WDYT? I'm not so sure about this one...

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

standard cmd ref structure.

we should not be following or trying to preserve it a cost of actual meaning and natural flow of the text. I don't know if this should go before or should be part of description, but introducing types and explaining the difference should be before explaining a particular type.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Would be tricky putting it in the Description before the basic explanation unless we remove the Types of metrics header and just make it into the first paragraph.

But for now I think it's fine actually, since this change only affects the index.md of both metrics and plots, which is kind of a placeholder cmd ref anyway. Other indices are used to explain concepts more than as cmd refs also, like for cache and remote 🙂 so we can be more flexible about these in terms of doc structure going fwd.

are produced by the model training or model evaluation code and serve as a way
to compare and pick the best performing experiment.

### Default metric files
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

should become part of the description

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

do we need a similar paragraph in plots?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It is part of the description.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

do we need a similar paragraph in plots?

No, plots require specifying a target/datafile

Copy link
Copy Markdown
Contributor

@jorgeorpinel jorgeorpinel Jun 1, 2020

Choose a reason for hiding this comment

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

p.s. this led me to discover the following issues: treeverse/dvc/issues/3925 and treeverse/dvc/issues/3924

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

it overlaps a lot:

For example:

Metric files can be added to dvc.yaml with the --metrics (-m) or
--metrics-no-cache (-M) options of dvc run, or manually to the metrics
section of a stage in dvc.yaml:

and

This kind of metrics can be defined with the -m (--metrics) and -M
(--metrics-no-cache) options of dvc run.

Why can't we make it a single paragraph?

anyway, probably not a big deal .. we can keep it like this for now

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

do we need a similar paragraph in plots?

No, plots require specifying a target/datafile

Actually probably we'll need it but this will be done as a result of resolving treeverse/dvc/issues/3925 (I'll keep track).

it overlaps a lot:

Yeah I didn't notice before. Removed redundancy in 47ad311 (part of #1382)!

Why can't we make it a single paragraph?

I could but would probably want to completely remove the dvc.yaml example in that case and the cache:false note. Also not sure it makes sense to have this info before the Supported file formats section. No strong opinion, I'll do it if you think it's best. Please lmk

Comment thread content/docs/command-reference/metrics/show.md Outdated
Comment thread content/docs/command-reference/plots/index.md Outdated
Comment thread content/docs/command-reference/plots/index.md Outdated
@jorgeorpinel jorgeorpinel temporarily deployed to dvc-landing-metr-1-0-pga0r0rsj June 1, 2020 05:56 Inactive
@jorgeorpinel jorgeorpinel temporarily deployed to dvc-landing-metr-1-0-pga0r0rsj June 1, 2020 06:34 Inactive
@jorgeorpinel jorgeorpinel temporarily deployed to dvc-landing-metr-1-0-pga0r0rsj June 1, 2020 09:36 Inactive
@jorgeorpinel jorgeorpinel requested a review from shcheklein June 1, 2020 16:02
@shcheklein shcheklein merged commit aefba93 into master Jun 1, 2020
@jorgeorpinel jorgeorpinel deleted the metr-1.0 branch June 6, 2020 08:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants