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 - plotting for multiple revisions initial #3577

Merged
merged 102 commits into from
May 1, 2020

Conversation

pared
Copy link
Contributor

@pared pared commented Apr 2, 2020

  • ❗ I have followed the Contributing to DVC checklist.

  • πŸ“– 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.

  • ❌ I will check DeepSource, CodeClimate, and other sanity checks below. (We consider them recommendatory and don't expect everything to be addressed. Please fix things that actually improve code or fix bugs.)

Thank you for the contribution - we'll try to review it as soon as possible. πŸ™

Next stage of #3569

@pared pared force-pushed the 3409_diff branch 3 times, most recently from 76a42af to 25f606b Compare April 3, 2020 14:15
@pared pared mentioned this pull request Apr 3, 2020
3 tasks
@pared pared force-pushed the 3409_diff branch 3 times, most recently from 33abe3e to d06056b Compare April 10, 2020 09:04
@pared pared marked this pull request as ready for review April 10, 2020 15:04
@pared pared changed the title [WIP] Metrics - plotting for multiple revisions initial Metrics - plotting for multiple revisions initial Apr 10, 2020
@pared pared requested review from efiop, pmrowla and skshetry April 10, 2020 15:18
Copy link
Contributor

@pmrowla pmrowla left a comment

Choose a reason for hiding this comment

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

Looks good for the most part, just had the one question about the 3.5 test issue

dvc/command/plot.py Outdated Show resolved Hide resolved
tests/func/test_plot.py Outdated Show resolved Hide resolved
dvc/repo/plot.py Outdated Show resolved Hide resolved
dvc/command/plot.py Outdated Show resolved Hide resolved
dvc/command/plot.py Outdated Show resolved Hide resolved
dvc/plot.py Outdated Show resolved Hide resolved
dvc/repo/plot.py Outdated Show resolved Hide resolved
dvc/repo/plot.py Outdated Show resolved Hide resolved
dvc/repo/plot.py Outdated Show resolved Hide resolved
dvc/repo/plot.py Outdated Show resolved Hide resolved
dvc/repo/plot.py Outdated Show resolved Hide resolved
help="Print plot content to stdout",
)
plot_diff_parser.add_argument(
"--no-csv-header",
Copy link
Member

Choose a reason for hiding this comment

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

@dmpetrov @pared can we name it just --no-header? simpler and covers TSV case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree here, @dmpetrov ?

dvc/repo/plot/data.py Outdated Show resolved Hide resolved
dvc/repo/plot/template.py Outdated Show resolved Hide resolved
dvc/repo/plot/template.py Outdated Show resolved Hide resolved
dvc/repo/plot/template.py Outdated Show resolved Hide resolved
dvc/repo/plot/data.py Outdated Show resolved Hide resolved
dvc/command/plot.py Outdated Show resolved Hide resolved
Copy link
Contributor

@efiop efiop left a comment

Choose a reason for hiding this comment

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

πŸ”₯ πŸ”₯ πŸ”₯

@efiop efiop merged commit e553511 into iterative:master May 1, 2020
dvc/command/plot.py Show resolved Hide resolved
dvc/command/plot.py Outdated Show resolved Hide resolved
"--template",
nargs="?",
default=None,
help="File to be injected with data.",
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe link to some doc where we explain templates? Something like

        help="Vega specification template file to inject with the data. See man.dvc.org/...",

See conversation in #3577 (comment)

Copy link
Contributor

Choose a reason for hiding this comment

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

Same in line 192

dvc/repo/plot/data.py Show resolved Hide resolved
Copy link
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.

A few post-merge ideas and questions @pared ☝️
Also possibly pending, #3577 (comment)

I can send a PR but if you could answer my questions when you can, that'll help me.

@pared pared mentioned this pull request May 7, 2020
jorgeorpinel added a commit that referenced this pull request May 7, 2020
efiop pushed a commit that referenced this pull request May 12, 2020
* plot: update datafile desc.

* plot: link to cmd ref in output

* plot: update --stdout desc
per #3577 (review)

* plot: a few more output string updates
per #3759 (review)
and iterative/dvc.org#1283
@pared pared deleted the 3409_diff branch January 4, 2022 10:12
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