From 52fe76cefcc7a1eafe6f8a11b5863a64425b5e26 Mon Sep 17 00:00:00 2001 From: Ruslan Kuprieiev Date: Mon, 5 Oct 2020 19:37:34 +0300 Subject: [PATCH] metrics: diff: make table more intuitive (#4655) * metrics: diff: make table more intuitive Currently we have a "diff not supported" marker in the table, that is used whenever dvc is not able to diff the values (e.g. no old one, no new one or format error). It is quite ugly and it is much more intuitive to just show both old and new values and now show that marker at all, as if there is no old/new value it is intuitive that we can't calculate the change. Next step is to add old/new "bad format"/etc markers for unsupported types or malformed/missing metric files. E.g., before: ``` Path Metric Value Change metrics.json foo 1.2 diff not supported ``` after this PR: ``` Path Metric Old New Change metrics.json foo None 1.2 ``` * metrics/params: use em dash in diff table --- dvc/command/metrics.py | 19 ++------ dvc/utils/diff.py | 2 +- tests/func/metrics/test_diff.py | 2 +- tests/unit/command/test_metrics.py | 78 +++++++++++------------------- tests/unit/command/test_params.py | 8 +-- 5 files changed, 39 insertions(+), 70 deletions(-) diff --git a/dvc/command/metrics.py b/dvc/command/metrics.py index 3c22a0321..9daf5984a 100644 --- a/dvc/command/metrics.py +++ b/dvc/command/metrics.py @@ -76,7 +76,7 @@ class CmdMetricsShow(CmdMetricsBase): return 0 -def _show_diff(diff, markdown=False, no_path=False, old=False, precision=None): +def _show_diff(diff, markdown=False, no_path=False, precision=None): from collections import OrderedDict from dvc.utils.diff import table @@ -96,18 +96,14 @@ def _show_diff(diff, markdown=False, no_path=False, old=False, precision=None): for metric, change in sorted_mdiff.items(): row = [] if no_path else [fname] row.append(metric) - if old: - row.append(_round(change.get("old"))) + row.append(_round(change.get("old"))) row.append(_round(change["new"])) - row.append(_round(change.get("diff", "diff not supported"))) + row.append(_round(change.get("diff"))) rows.append(row) header = [] if no_path else ["Path"] header.append("Metric") - if old: - header.extend(["Old", "New"]) - else: - header.append("Value") + header.extend(["Old", "New"]) header.append("Change") return table(header, rows, markdown) @@ -133,7 +129,6 @@ class CmdMetricsDiff(CmdMetricsBase): diff, self.args.show_md, self.args.no_path, - self.args.old, precision=self.args.precision, ) if table: @@ -275,12 +270,6 @@ def add_parser(subparsers, parent_parser): default=False, help="Show tabulated output in the Markdown format (GFM).", ) - metrics_diff_parser.add_argument( - "--old", - action="store_true", - default=False, - help="Show old metric value.", - ) metrics_diff_parser.add_argument( "--no-path", action="store_true", diff --git a/dvc/utils/diff.py b/dvc/utils/diff.py index 219691359..d12267f64 100644 --- a/dvc/utils/diff.py +++ b/dvc/utils/diff.py @@ -97,7 +97,7 @@ def table(header, rows, markdown=False): tablefmt="github" if markdown else "plain", disable_numparse=True, # None will be shown as "" by default, overriding - missingval="None", + missingval="—", ) if markdown: diff --git a/tests/func/metrics/test_diff.py b/tests/func/metrics/test_diff.py index 8d00c2d81..94b4cc255 100644 --- a/tests/func/metrics/test_diff.py +++ b/tests/func/metrics/test_diff.py @@ -185,7 +185,7 @@ def test_metrics_diff_cli(tmp_dir, scm, dvc, run_copy_metrics, caplog, capsys): _gen(3.45678910111213) caplog.clear() - assert main(["metrics", "diff", "HEAD~2", "--old"]) == 0 + assert main(["metrics", "diff", "HEAD~2"]) == 0 (info,) = [ msg for name, level, msg in caplog.record_tuples diff --git a/tests/unit/command/test_metrics.py b/tests/unit/command/test_metrics.py index 1e8650556..aede55a30 100644 --- a/tests/unit/command/test_metrics.py +++ b/tests/unit/command/test_metrics.py @@ -24,7 +24,6 @@ def test_metrics_diff(dvc, mocker): "target2", "--show-md", "--no-path", - "--old", "--precision", "10", ] @@ -51,8 +50,8 @@ def test_metrics_show_json_diff(): {"metrics.json": {"a.b.c": {"old": 1, "new": 2, "diff": 3}}} ) == textwrap.dedent( """\ - Path Metric Value Change - metrics.json a.b.c 2 3""" + Path Metric Old New Change + metrics.json a.b.c 1 2 3""" ) @@ -61,8 +60,8 @@ def test_metrics_show_raw_diff(): {"metrics": {"": {"old": "1", "new": "2"}}} ) == textwrap.dedent( """\ - Path Metric Value Change - metrics 2 diff not supported""" + Path Metric Old New Change + metrics 1 2 —""" ) @@ -71,8 +70,8 @@ def test_metrics_diff_no_diff(): {"other.json": {"a.b.d": {"old": "old", "new": "new"}}} ) == textwrap.dedent( """\ - Path Metric Value Change - other.json a.b.d new diff not supported""" + Path Metric Old New Change + other.json a.b.d old new —""" ) @@ -85,8 +84,8 @@ def test_metrics_diff_new_metric(): {"other.json": {"a.b.d": {"old": None, "new": "new"}}} ) == textwrap.dedent( """\ - Path Metric Value Change - other.json a.b.d new diff not supported""" + Path Metric Old New Change + other.json a.b.d — new —""" ) @@ -95,8 +94,8 @@ def test_metrics_diff_deleted_metric(): {"other.json": {"a.b.d": {"old": "old", "new": None}}} ) == textwrap.dedent( """\ - Path Metric Value Change - other.json a.b.d None diff not supported""" + Path Metric Old New Change + other.json a.b.d old — —""" ) @@ -143,14 +142,14 @@ def test_metrics_diff_precision(): assert _show_diff(diff) == textwrap.dedent( """\ - Path Metric Value Change - other.json a.b 0.76543 0.64198""" + Path Metric Old New Change + other.json a.b 0.12346 0.76543 0.64198""" ) assert _show_diff(diff, precision=10) == textwrap.dedent( """\ - Path Metric Value Change - other.json a.b 0.7654321012 0.6419754012""" + Path Metric Old New Change + other.json a.b 0.1234567 0.7654321012 0.6419754012""" ) @@ -165,18 +164,18 @@ def test_metrics_diff_sorted(): } ) == textwrap.dedent( """\ - Path Metric Value Change - metrics.yaml a.b.c 2 1 - metrics.yaml a.d.e 4 1 - metrics.yaml x.b 6 1""" + Path Metric Old New Change + metrics.yaml a.b.c 1 2 1 + metrics.yaml a.d.e 3 4 1 + metrics.yaml x.b 5 6 1""" ) def test_metrics_diff_markdown_empty(): assert _show_diff({}, markdown=True) == textwrap.dedent( """\ - | Path | Metric | Value | Change | - |--------|----------|---------|----------| + | Path | Metric | Old | New | Change | + |--------|----------|-------|-------|----------| """ ) @@ -193,11 +192,11 @@ def test_metrics_diff_markdown(): markdown=True, ) == textwrap.dedent( """\ - | Path | Metric | Value | Change | - |--------------|----------|---------|--------------------| - | metrics.yaml | a.b.c | 2 | 1 | - | metrics.yaml | a.d.e | 4 | 1 | - | metrics.yaml | x.b | 6 | diff not supported | + | Path | Metric | Old | New | Change | + |--------------|----------|-------|-------|----------| + | metrics.yaml | a.b.c | 1 | 2 | 1 | + | metrics.yaml | a.d.e | 3 | 4 | 1 | + | metrics.yaml | x.b | 5 | 6 | — | """ ) @@ -214,29 +213,10 @@ def test_metrics_diff_no_path(): no_path=True, ) == textwrap.dedent( """\ - Metric Value Change - a.b.c 2 1 - a.d.e 4 1 - x.b 6 1""" - ) - - -def test_metrics_diff_with_old(): - assert _show_diff( - { - "metrics.yaml": { - "x.b": {"old": 5, "new": 6, "diff": 1}, - "a.d.e": {"old": 3, "new": 4, "diff": 1}, - "a.b.c": {"old": 1, "new": 2, "diff": 1}, - } - }, - old=True, - ) == textwrap.dedent( - """\ - Path Metric Old New Change - metrics.yaml a.b.c 1 2 1 - metrics.yaml a.d.e 3 4 1 - metrics.yaml x.b 5 6 1""" + Metric Old New Change + a.b.c 1 2 1 + a.d.e 3 4 1 + x.b 5 6 1""" ) diff --git a/tests/unit/command/test_params.py b/tests/unit/command/test_params.py index de55499cd..4c61c60ec 100644 --- a/tests/unit/command/test_params.py +++ b/tests/unit/command/test_params.py @@ -70,7 +70,7 @@ def test_params_diff_new(): ) == textwrap.dedent( """\ Path Param Old New - params.yaml a.b.d None new""" + params.yaml a.b.d — new""" ) @@ -80,7 +80,7 @@ def test_params_diff_deleted(): ) == textwrap.dedent( """\ Path Param Old New - params.yaml a.b.d old None""" + params.yaml a.b.d old —""" ) @@ -148,8 +148,8 @@ def test_params_diff_markdown(): """\ | Path | Param | Old | New | |-------------|---------|-------|-------| - | params.yaml | a.b.c | 1 | None | - | params.yaml | a.d.e | None | 4 | + | params.yaml | a.b.c | 1 | — | + | params.yaml | a.d.e | — | 4 | | params.yaml | x.b | 5 | 6 | """ )