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

metrics: introduce diff #3051

Merged
merged 1 commit into from
Jan 14, 2020
Merged

metrics: introduce diff #3051

merged 1 commit into from
Jan 14, 2020

Conversation

efiop
Copy link
Contributor

@efiop efiop commented Jan 4, 2020

This first implementation is based on existing dvc metrics show
functionality, which has type and xpath support to determine
specific values in your file that you are interested in. That makes
comparing pretty straightforward to implement.

Related to #2995

TODO:

  • Possibly multiple fields in the metrics file Multiple metrics in one file #1682 to match what has been described in the original issue.
  • Reconsider CLI flags (e.g. old-rev/new-rev are not the nicest looking). UPDATE: for now following the same scheme as in dvc diff
  • Probably better to show the key for a specific metric field in the output, but that doesn't really match up with our current metrics show behaviour in case of type/xpath being specified.
  • If type/spath are not specified, we could detect the type and diff the whole csv/etc/json. UPDATE: for now keeping it explicit, same as in dvc metrics show.

  • ❗ Have you followed the guidelines in the Contributing to DVC list?

  • 📖 Check this box if this PR does not require documentation updates, or if it does and you have created a separate PR in dvc.org with such updates (or at least opened an issue about it in that repo). Please link below to your PR (or issue) in the dvc.org repo.

iterative/dvc.org#921

  • ❌ Have you checked DeepSource, CodeClimate, and other sanity checks below? We consider their findings recommendatory and don't expect everything to be addressed. Please review them carefully and fix those that actually improve code or fix bugs.

Thank you for the contribution - we'll try to review it as soon as possible. 🙏

@efiop efiop changed the title [WIP] metrics: introduce diff method [WIP] metrics: introduce diff Jan 4, 2020
@efiop efiop force-pushed the 2995 branch 16 times, most recently from a1bc1a3 to 0f13fc8 Compare January 10, 2020 06:00
@efiop efiop force-pushed the 2995 branch 7 times, most recently from 8a9f5fd to 947571e Compare January 13, 2020 02:03
@efiop efiop changed the title [WIP] metrics: introduce diff metrics: introduce diff Jan 13, 2020
@efiop efiop marked this pull request as ready for review January 13, 2020 02:41
@efiop efiop changed the title metrics: introduce diff [WIP] metrics: introduce diff Jan 13, 2020
@efiop efiop force-pushed the 2995 branch 3 times, most recently from 96b651c to 1931f3b Compare January 13, 2020 04:45
This first implementation is based on existing `dvc metrics show`
functionality, but is also able to auto-detect json to properly
show diff for it.
@efiop efiop changed the title [WIP] metrics: introduce diff metrics: introduce diff Jan 13, 2020
table.set_chars(("", "", "", ""))
table.set_deco(0)

rows = [["Path", "Metric", "Value", "Change"]]
Copy link
Contributor Author

@efiop efiop Jan 14, 2020

Choose a reason for hiding this comment

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

@dmpetrov asked for this format specifically, so don't be surprised that we don't show old value here :)

@efiop efiop merged commit 9cafef1 into iterative:master Jan 14, 2020
@jorgeorpinel jorgeorpinel mentioned this pull request Jan 27, 2020
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.

2 participants