metrics & plots: help output improvements by nik123 · Pull Request #3963 · treeverse/dvc

nik123 · 2020-06-05T16:06:01Z

📖 If this PR requires documentation updates, I have created a separate PR (or issue, at least) in dvc.org and linked it here. If the CLI API is changed, I have updated tab completion scripts.
cmd: pending plots changes by 1.0 release dvc.org#1414

It seems that changes might be covered in treeverse/dvc.org#1382. The list of required changes in docs:

Few param names in docs are outdated: --xlab -> --x-label and --ylab -> --y-label
dvc metrics diff: help for targets param should be changed to Limit command scope to these metric files. Using -R, directories to search metric files in can also be given.
dvc metrics show: help for targets param should be changed to Limit command scope to these metric files. Using -R, directories to search metric files in can also be given.
dvc plots diff -h: command description should be updated (see discussion in comments).

nik123 · 2020-06-05T16:16:03Z

Few param names in docs are outdated: --xlab -> --x-label and --ylab -> --y-label but it seems it is already covered here https://github.com/iterative/dvc.org/pull/1382/files#r434965944

efiop · 2020-06-05T23:52:23Z

        "--targets",
        nargs="*",
        help=(
-            "Metric files or directories (see -R) to show diff for. "


Actually, in this case -R works, so we should keep it 🙂 But probably need to rephrase it a bit, because it is about directories containing metric files.

Updated the message. May still be badly phrased though

I think the new text is accurate (taken from older commands like push/pull). Should we also apply it to metrics show though? I think so!

Changed text in metrics show. Please, check it out.

efiop · 2020-06-05T23:53:13Z

    fix_subparsers(plots_subparsers)

-    SHOW_HELP = "Generate a plots image file from a metrics file."
+    SHOW_HELP = "Generate plot from a metrics file."


Actually, it can generate multiple plots from multiple metrics files.

jorgeorpinel · 2020-06-07T02:02:25Z

already covered here https://github.com/iterative/dvc.org/pull/1382/files#r434965944

Yes, some of it but maybe this PR will have even more changes so could we maybe restore the PR template with the check boxes so we don't forget to apply the same changes to docs afterwards?

It could be done directly in treeverse/dvc.org/pull/1382 if it's still in progress once this is approved.

jorgeorpinel · 2020-06-07T02:43:58Z

 def add_parser(subparsers, parent_parser):
    PLOTS_HELP = (
-        "Generating plots for metrics stored in structured files "
+        "Commands to visualize and compare plot metrics in structured files "


Thanks for this change. We may be changing it further per this treeverse/dvc.org#1382 (comment). Please check the resolution tomorrow or so.

jorgeorpinel

So far so good, thanks! (Just left a couple small notes ☝️).

I'm not opposed to merging but there's a few more things related to metrics output strings we could address here or separately:

Lots of unnecessary periods in the non-sentence descriptions of command options.
Similar updates apply to plots modify now that it has been added, also based on https://github.com/iterative/dvc.org/pull/1382/files?file-filters%5B%5D=.md#diff-b6540815158031bd570b61c0cb46c19f but arguable, of course. For example:
- PLOTS_MODIFY_HELP = "Modify display properties of plot metric files."
- target arg help: `"Metric file to set props to" (no period)
- for --unset: "Unset one or more display properties."
Additionally, it would be nice to reorder the command parameters to match the order in the Options sections of the corresponding cmd refs (as ordered in #1382)

nik123 · 2020-06-07T07:22:07Z

already covered here https://github.com/iterative/dvc.org/pull/1382/files#r434965944

Yes, some of it but maybe this PR will have even more changes so could we maybe restore the PR template with the check boxes so we don't forget to apply the same changes to docs afterwards?

It could be done directly in iterative/dvc.org/pull/1382 if it's still in progress once this is approved.

@jorgeorpinel I added list of required documentation changes into original post. I will create separate PR if treeverse/dvc.org#1382 will be resolved before this one.

pared

2 comments, I like including metavar.

pared · 2020-06-08T12:16:45Z

-        "Plot differences in metrics between commits in the DVC "
-        "repository, or between the last commit and the workspace."
-    )
+    PLOTS_DIFF_HELP = "Plot differences in metrics between commits."


I think phrasing here (in original DIFF_HELP too) is a bit off. We never plot differences. Plots exist alongside, either on the same plot (default, scatter) or on separate canvases (confusion matrix).

In the case of metrics, we do diff them by subtracting. Word diff suggests visualizing some kind of difference between metrics, which is not happening in plots.
I am not sure whether we should treat the plot in a similar way we do with metrics.

@pared but the command is called diff :) A few lines on the same plot could also be considered as diff, that's what we do and why the command is called dvc plots diff. As you've mentioned, we don't really plot all plots in the same image, but rather we show 1 image per 1 plot with multiple lines in it, and that is how we visualise a diff for a particular plot.

pared · 2020-06-08T12:19:46Z

+        "revisions",
+        nargs="*",
+        default=None,
+        help="Git commits to plot from/to",


We do not write plots to commits. I might miss something, why this to?

That was my fault. Our latest version is

Git commits to find metrics to compare

per treeverse/dvc.org#1382 (review)

pared

One minor thingy

pared · 2020-06-09T10:17:05Z

-        "Plot differences in metrics between commits in the DVC "
-        "repository, or between the last commit and the workspace."
+        "Show multiple versions of plot metrics "
+        "by plotting them in a single image."


It's not always single image (confusion matrix requires to plot few of them). I think Show multiple versions of plot metircs. Would be fine

Shame we lost the info about "between commits in the DVC repository, or between the last commit and the workspace." 🙁

What about "Show multiple versions of plot metrics from different commits"?

Or we should go back to "Plot differences in metrics between commits in the DVC repository, or between the last commit and the workspace."

Ok, I guess I caused some confusion here, by reviewing even the base version. @nik123 I am sorry about that. @jorgeorpinel could you take look? In docs, we have:

This command visualize difference between metrics among experiments in the repository history.

That sounds about right.

@nik123 I am for that, let's ask @jorgeorpinel for the green light.

@pared But you are the author of this feature. Isn't single image completely wrong?

@efiop not quite, even for confusion matrix one could argue that we are dealing with single image. Take a look at this example What we have here is three plots on one image, thats why I believe Jorge is right, and I was wrong in the beginning.

It seems we almost have an agreement. @jorgeorpinel what are your thoughts about last proposed text?

Well, I lost track of this one until now oops, sorry! The text you came up with is good 🙂

# Conflicts: # dvc/command/plots.py

jorgeorpinel

Thanks for the updates @nik123! And sorry for the delay to come back. Here are a few more minor suggestions after a final pass. Probably you could just commit them from GH

jorgeorpinel · 2020-06-10T05:22:04Z

+        "-o",
+        "--out",
+        default=None,
+        help="Destination path to save plots to.",


Suggested change

help="Destination path to save plots to.",

help="Destination path to save plots to",

@jorgeorpinel Is it really worth bothering with dots in this PR? We have dots all over the place, I don't really see a point in blocking this PR just because of it: it makes it even less consistent.

Co-authored-by: Jorge Orpinel <jorgeorpinel@users.noreply.github.com>

efiop · 2020-06-14T17:40:31Z

Thank you @nik123 ! 🙏 Sorry for the delay, we'll take it from here.

* cmd ref: metrics diff: targets desc updated per treeverse/dvc#3963 * cmd ref: metrics show: targets desc updated per treeverse/dvc#3963

metrics & plots: help output improvements

22c9e5f

efiop requested review from jorgeorpinel and pared June 5, 2020 20:42

efiop reviewed Jun 5, 2020

View reviewed changes

nik123 added 2 commits June 6, 2020 13:30

metrics & plots: ui: updated help messages

6e09ff8

Merge branch 'master' into MetricsAndPlotsHelpOutputImprovements

9b3a34e

nik123 requested a review from efiop June 6, 2020 13:14

jorgeorpinel reviewed Jun 7, 2020

View reviewed changes

jorgeorpinel mentioned this pull request Jun 7, 2020

Regular updates & plots 1.0 update treeverse/dvc.org#1382

Merged

nik123 added 4 commits June 7, 2020 14:47

refactor: removed redundant parentheses

22250ab

metrics: ui: updated 'targets' description in 'metrics show'

b5e1d73

metrics: ui: reordered params to match the order in cmd ref

051bc76

plots: ui: params modified to match cmd ref

b9e7e44

efiop requested a review from jorgeorpinel June 8, 2020 08:44

pared suggested changes Jun 8, 2020

View reviewed changes

plots: ui: updated diff help

41d0639

efiop requested a review from pared June 9, 2020 09:57

pared reviewed Jun 9, 2020

View reviewed changes

Merge branch 'master' into MetricsAndPlotsHelpOutputImprovements

6bd01da

# Conflicts: # dvc/command/plots.py

jorgeorpinel mentioned this pull request Jun 9, 2020

cmd: pending plots changes by 1.0 release treeverse/dvc.org#1414

Closed

6 tasks

jorgeorpinel reviewed Jun 10, 2020

View reviewed changes

efiop and others added 4 commits June 10, 2020 14:09

Update dvc/command/plots.py

62bf429

Co-authored-by: Jorge Orpinel <jorgeorpinel@users.noreply.github.com>

Update dvc/command/plots.py

e9bde91

Co-authored-by: Jorge Orpinel <jorgeorpinel@users.noreply.github.com>

Update dvc/command/plots.py

a49d3ff

Co-authored-by: Jorge Orpinel <jorgeorpinel@users.noreply.github.com>

Update dvc/command/plots.py

864b977

Co-authored-by: Jorge Orpinel <jorgeorpinel@users.noreply.github.com>

efiop and others added 3 commits June 10, 2020 14:09

Update dvc/command/plots.py

09779d6

Co-authored-by: Jorge Orpinel <jorgeorpinel@users.noreply.github.com>

Update dvc/command/plots.py

d10ceb4

Co-authored-by: Jorge Orpinel <jorgeorpinel@users.noreply.github.com>

Merge branch 'master' into MetricsAndPlotsHelpOutputImprovements

e2b0c6b

efiop merged commit f7348af into treeverse:master Jun 14, 2020

nik123 deleted the MetricsAndPlotsHelpOutputImprovements branch June 17, 2020 15:53

nik123 added a commit to nik123/dvc.org that referenced this pull request Jun 17, 2020

cmd ref: metrics 1.0 update

c8a0640

* cmd ref: metrics diff: targets desc updated per treeverse/dvc#3963 * cmd ref: metrics show: targets desc updated per treeverse/dvc#3963

nik123 mentioned this pull request Jun 17, 2020

cmd ref: metrics 1.0 update treeverse/dvc.org#1444

Merged

shcheklein pushed a commit to treeverse/dvc.org that referenced this pull request Jun 17, 2020

cmd ref: metrics 1.0 update (#1444)

c9354bc

* cmd ref: metrics diff: targets desc updated per treeverse/dvc#3963 * cmd ref: metrics show: targets desc updated per treeverse/dvc#3963

imhardikj mentioned this pull request Nov 15, 2020

cmd: example on modifying any outputs into a plots treeverse/dvc.org#1946

Merged

2 tasks

	help="Destination path to save plots to.",
	help="Destination path to save plots to",

Conversation

nik123 commented Jun 5, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nik123 commented Jun 5, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jorgeorpinel commented Jun 7, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jorgeorpinel left a comment

Choose a reason for hiding this comment

Uh oh!

nik123 commented Jun 7, 2020

Uh oh!

pared left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pared left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jorgeorpinel left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

efiop Jun 10, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

efiop commented Jun 14, 2020

Uh oh!

Reviewers

Assignees

Labels

nik123 commented Jun 5, 2020 •

edited

Loading

efiop Jun 10, 2020 •

edited

Loading