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

params/metrics/exp diff: consistent output #5515

Closed
jorgeorpinel opened this issue Feb 24, 2021 · 19 comments
Closed

params/metrics/exp diff: consistent output #5515

jorgeorpinel opened this issue Feb 24, 2021 · 19 comments
Labels
diff/show Related to the diff/show feature enhancement Enhances DVC good first issue p3-nice-to-have It should be done this or next sprint ui user interface / interaction

Comments

@jorgeorpinel
Copy link
Contributor

Extracted from iterative/dvc.org#2200 (comment)

  • metrics diff currently shows old, new, change
  • params diff currently shows old, new
  • exp diff currently shows new, change

Should they all be Old, New, Change? (Possibly with some flag to skip Old or Change)

@jorgeorpinel jorgeorpinel added enhancement Enhances DVC ui user interface / interaction labels Feb 24, 2021
@jorgeorpinel
Copy link
Contributor Author

cc @dberenbaum

@dberenbaum
Copy link
Collaborator

Thanks, @jorgeorpinel! Let's refer this back to #5392.

Should they all be Old, New, Change?

👍

@jorgeorpinel

This comment has been minimized.

@jorgeorpinel
Copy link
Contributor Author

jorgeorpinel commented Feb 25, 2021

BTW @pmrowla do you know if we've considered merging dvc params/metrics/exp diff into a single command (e.g. dvc compare)? Or maybe as flags of dvc diff.

@pmrowla
Copy link
Contributor

pmrowla commented Feb 25, 2021

@jorgeorpinel the idea has come up before as something that we could consider doing in the future, but not seriously considered yet

@dberenbaum

This comment has been minimized.

@daavoo daavoo added the diff/show Related to the diff/show feature label Jul 20, 2021
@daavoo
Copy link
Contributor

daavoo commented Jul 21, 2021

Was about to comment this in #5392 (haven't seen any other related issues). I think that the terms Old / New used in diff sometimes could be confusing, given that users can pass refs in any order.

For example they could run dvc metrics diff {newer commit hash} {older commit hash} and the table will show the values of the newer commit under the column Old.

@dberenbaum
Copy link
Collaborator

cc @skshetry

I'd prefer having the ref names instead of Old and New.

@daavoo
Copy link
Contributor

daavoo commented Oct 29, 2021

Two questions:

A) Should Change be included by default ??

I might be biased but, in my personal experience (and current docs examples), the "Change" value is kind of awkward most of the times.

I would vote for hiding it by default and making it optional.

B) Should there be an option to hide a_rev (previously "Old") at all?

IMO the only case where it could make sense to not show a_rev is when calling dvc {X} diff {b_rev} (where a_rev is set to HEAD by default.)

Maybe instead of a flag for opting out a_rev, we could just hide it when equals to HEAD.

@dberenbaum
Copy link
Collaborator

@daavoo @jorgeorpinel Is there anything left to do on this issue?

@daavoo
Copy link
Contributor

daavoo commented Nov 15, 2021

@daavoo @jorgeorpinel Is there anything left to do on this issue?

Showing Change in dvc params diff (If we agree that Change should be showed by default)

@jorgeorpinel
Copy link
Contributor Author

Not sure, I'd expect some PR to close this at some point but feel free to do so manually when you think it's addressed. Thanks

@dberenbaum
Copy link
Collaborator

Params are not always numeric, so I'm not sure what should be shown in Change. Maybe it's a boolean for non-numeric params?

I agree that Change is not generally something I find that useful, but I also don't see it as really harmful.

@daavoo
Copy link
Contributor

daavoo commented Nov 15, 2021

Params are not always numeric, so I'm not sure what should be shown in Change. Maybe it's a boolean for non-numeric params?

We are showing Change in the params section of dvc exp diff treating them as scalars https://dvc.org/doc/command-reference/exp/diff#examples 🤷

@dberenbaum
Copy link
Collaborator

Ah, okay, it's handled like:

$ dvc exp diff
Path         Param    HEAD    workspace    Change
params.yaml  foo      a       b            diff not supported

I'd vote to keep Change since I see little harm and it seems like a less breaking change, but I don't feel that strongly about it.

@daavoo
Copy link
Contributor

daavoo commented Nov 15, 2021

Ah, okay, it's handled like:

$ dvc exp diff
Path         Param    HEAD    workspace    Change
params.yaml  foo      a       b            diff not supported

I'd vote to keep Change since I see little harm and it seems like a less breaking change, but I don't feel that strongly about it.

We could improve the non scalar support by replacing the 'diff not supported' with bool resulting of direct comparisson

@daavoo
Copy link
Contributor

daavoo commented Aug 1, 2022

To clarify, what's left to close the issue is:

  • Showing Change in dvc params diff

@mesejo
Copy link

mesejo commented Jul 20, 2023

If this is still open, I would like to take it.

@dberenbaum dberenbaum added the p3-nice-to-have It should be done this or next sprint label Sep 27, 2023
@filippopellizzari
Copy link

If no one is currently working on this, I would like to take it.

@mattseddon mattseddon closed this as not planned Won't fix, can't repro, duplicate, stale Mar 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
diff/show Related to the diff/show feature enhancement Enhances DVC good first issue p3-nice-to-have It should be done this or next sprint ui user interface / interaction
Projects
None yet
Development

No branches or pull requests

7 participants