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.

Sign up for GitHub

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

Jump to bottom

cmd-ref: plots: flexible plots docs #3691

Closed
wants to merge 24 commits into from
Closed

cmd-ref: plots: flexible plots docs #3691

wants to merge 24 commits into from

Conversation

pared
Copy link
Contributor

@pared pared commented Jun 24, 2022 •
edited by jorgeorpinel

Related to: iterative/dvc#7477 (iterative/dvc#7086)

Docs for flexible plots.
Added templates command reference.
Partially solves #2956. - It seems to me that there is no point in differentiating between metrics/plots notation. Plots are no longer special types of output. Main point of the plots is to provide visualization. Visualizations can be created from any supported type of data, hence we only need to mention that there exists special type of output (plots) That can help with visualizations.

Do not merge before: iterative/dvc#7477 is merged.

In review app: https://dvc-org-7086-flexible-p-d0hfxv.herokuapp.com/doc/command-reference/plots

UPDATE: Jump down to #3691 (review).

@julieg18 julieg18 temporarily deployed to dvc-org-7086-flexible-p-d0hfxv June 24, 2022 15:26 Inactive
shcheklein
shcheklein reviewed Jun 25, 2022
View reviewed changes
content/docs/command-reference/plots/index.md Outdated
show Generate plots from target files or plots definitions from `dvc.yaml` file.
diff Show multiple versions of plot data by plotting them in a single image.
modify Modify display properties of data-series plot outputs (has no effect on image-type plots).
templates Write built-in plots templates to a directory (.dvc/plots by default).
Copy link
Member

Choose a reason for hiding this comment

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

would be better to have dvc plots templates (show available templates, including custom?), dvc plots template --dump <template-name> dumps it.

(feels like dumping a template is an aux command compared to show / diff and even modify)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can discuss and modify the usage, but I would prefer to avoid it in this PR.

shcheklein
shcheklein reviewed Jun 25, 2022
View reviewed changes
content/docs/command-reference/plots/index.md Outdated
train_classes.csv: predicted_class
test_classes.csv: predicted_class
title: Compare test vs train confusion matrix
template: confusion
Copy link
Member

Choose a reason for hiding this comment

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

how does diff look like in this case (unmergable plots)? can you show an example please? just trying to understand the consequences for VS Code / Studio

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here is an example for my sandbox project:
visualization

Should I generate this example for docs too?

Copy link
Member

Choose a reason for hiding this comment

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

Got it. I would say it's confusing to see rev as a title a bit. Not related to this PR obviously.

I think it's fine to have a simple case for these plots. I don't it's required or something.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, I will have to fix that. Regretfully, because in the beginning, I did not convert rev into an anchor, this field will have to stay with us for some time. I should probably be able to remove it from the subtitle though.

Copy link
Contributor

Choose a reason for hiding this comment

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

Do we think it's useful to include a diff example here (doesn't need to block the PR)?

shcheklein
shcheklein requested changes Jun 25, 2022
View reviewed changes
Copy link
Member

@shcheklein shcheklein left a comment

Choose a reason for hiding this comment

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

How does it affect the plots modify? can I pass to it some extra arguments?

shcheklein
shcheklein previously requested changes Jun 25, 2022
View reviewed changes
Copy link
Member

@shcheklein shcheklein left a comment

Choose a reason for hiding this comment

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

It feels we need somewhere a better description of the possible structure of the plots yaml section (table with examples)?

do we need to update the schema file for the dvc.yaml?

@jorgeorpinel jorgeorpinel added the ⌛ status: wait-core-merge Waiting for related product PR merge/release label Jun 25, 2022
@shcheklein shcheklein temporarily deployed to dvc-org-7086-flexible-p-d0hfxv June 25, 2022 02:44 Inactive
jorgeorpinel
jorgeorpinel reviewed Jun 25, 2022
View reviewed changes
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.

Stylistic changes to Synopsis... please apply to content/docs/command-reference/plots/templates.md as well 🙂

content/docs/command-reference/plots/index.md Outdated Show resolved Hide resolved
content/docs/command-reference/plots/index.md Outdated Show resolved Hide resolved
content/docs/command-reference/plots/index.md Outdated Show resolved Hide resolved
@pared
Copy link
Contributor Author

pared commented Jun 27, 2022

How does it affect the plots modify? can I pass to it some extra arguments?

For now plots modify work as they used to - for plot types of outputs. I didn't change the behavior not to bloat the existing change. There is also a question of whether modify should allow creating a new plot in dvc.yaml

@shcheklein shcheklein temporarily deployed to dvc-org-7086-flexible-p-d0hfxv June 27, 2022 09:32 Inactive
@shcheklein shcheklein temporarily deployed to dvc-org-7086-flexible-p-d0hfxv June 27, 2022 09:45 Inactive
@pared
Copy link
Contributor Author

pared commented Jun 27, 2022

do we need to update the schema file for the dvc.yaml?

@shcheklein
You mean to include plots in doc/user-guide/project-structure/pipelines-files?

@pared
Copy link
Contributor Author

pared commented Jun 27, 2022 •
edited by jorgeorpinel

Stylistic changes to Synopsis... please apply to content/docs/command-reference/plots/templates.md as well

Modified the templates.md synopsis (in #3697).

daavoo
daavoo reviewed Jun 27, 2022
View reviewed changes
Copy link
Contributor

@daavoo daavoo left a comment

Choose a reason for hiding this comment

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

Doesn't need to block this P.R. but would be nice to mention the new plots section also in https://dvc.org/doc/user-guide/project-structure/pipelines-files

@dberenbaum
Copy link
Contributor

@pared Is it possible to break into multiple PRs? iterative/dvc#7477 alone is a large number of changes. Do we need to combine that, #3295, and #2956?

@pared
Copy link
Contributor Author

pared commented Jun 28, 2022

@dberenbaum I think that we can split templates docs, thought I don't see why should we split #3295 and #2956 - the change in way of thinking about plots is part of #3295 so I think those two should be part of a single change.

@shcheklein shcheklein temporarily deployed to dvc-org-7086-flexible-p-d0hfxv June 28, 2022 10:49 Inactive
@shcheklein shcheklein temporarily deployed to dvc-org-7086-flexible-p-d0hfxv June 28, 2022 10:51 Inactive
@shcheklein shcheklein temporarily deployed to dvc-org-7086-flexible-p-d0hfxv June 28, 2022 10:55 Inactive
jorgeorpinel and others added 20 commits July 28, 2022 09:30
@jorgeorpinel @pared
Update content/docs/command-reference/plots/index.md
b363691
@pared
plots: examples: move to subcommands
3c1859f
@pared
plots: refactor top-level plots definition
ea041c4
@pared
plots: review refactor
1f55c1c
@jorgeorpinel @pared
Update content/docs/command-reference/plots/index.md
0db83d6
@jorgeorpinel @pared
ref: fix a link (2/2)
bc9a6a2 
per iterative#3691 (review)
@jorgeorpinel @pared
ref: remove concept of type of metrics
f28813a
@jorgeorpinel @pared
ref: term "plots files" (consistency)
ab9f138
@jorgeorpinel @pared
ref: wrap plots index usage block
6dfd730
@jorgeorpinel @pared
plots: top-level plots edits
92a8ebf
@jorgeorpinel @pared
plots: improve motivation for top-level plots
79aeef1 
per iterative#3691 (review)
@jorgeorpinel @pared
ref: edit plots show desc
3ebbcf0
@jorgeorpinel @pared
ref: return plot template examples from plots show to index
455af6f
@jorgeorpinel @pared
guide: move top-lv plot mention from stage entry to desc
30d8130
@jorgeorpinel @pared
ref: clean up new plots show examples
0e5699c
@jorgeorpinel @pared
ref: more copy edits around plots
1b0bbe0
@dberenbaum @pared
Update content/docs/user-guide/project-structure/dvcyaml-files.md
8e81ce2
@dberenbaum @pared
Update content/docs/command-reference/plots/index.md
9d1f704
@dberenbaum @pared
Update content/docs/command-reference/plots/index.md
a1730e2
@dberenbaum @pared
Update content/docs/command-reference/plots/index.md
3247e8b
@shcheklein shcheklein temporarily deployed to dvc-org-7086-flexible-p-d0hfxv July 28, 2022 07:49 Inactive
dberenbaum added a commit that referenced this pull request Jul 28, 2022
@restyled-io @pared
@jorgeorpinel @dberenbaum @restyled-commits
Restyle cmd-ref: plots: flexible plots docs (#3819)
c72dafc 
* cmd-ref: plots: flexible plots docs

Related: iterative/dvc#7477
Related: #2956

* Update content/docs/user-guide/project-structure/dvcyaml-files.md

* Apply suggestions from code review

* Update content/docs/command-reference/plots/index.md

* plots: examples: move to subcommands

* plots: refactor top-level plots definition

* plots: review refactor

* Update content/docs/command-reference/plots/index.md

* ref: fix a link (2/2)
per #3691 (review)

* ref: remove concept of type of metrics

* ref: term "plots files" (consistency)

* ref: wrap `plots index` usage block

* plots: top-level plots edits

* plots: improve motivation for top-level plots
per #3691 (review)

* ref: edit `plots show` desc

* ref: return plot template examples from `plots show` to index

* guide: move top-lv plot mention from stage entry to desc

* ref: clean up new `plots show` examples

* ref: more copy edits around plots

* Update content/docs/user-guide/project-structure/dvcyaml-files.md

* Update content/docs/command-reference/plots/index.md

* Update content/docs/command-reference/plots/index.md

* Update content/docs/command-reference/plots/index.md

* Restyled by prettier

Co-authored-by: Paweł Redzyński <pawelredzynski@gmail.com>
Co-authored-by: Jorge Orpinel <jorgeorpinel@users.noreply.github.com>
Co-authored-by: Jorge Orpinel Perez <jorge@orpinel.com>
Co-authored-by: Dave Berenbaum <dave.berenbaum@gmail.com>
Co-authored-by: Restyled.io <commits@restyled.io>
Co-authored-by: Dave Berenbaum <dave@iterative.ai>
@dberenbaum
Copy link
Contributor

Closing as merged in #3819

@dberenbaum dberenbaum closed this Jul 28, 2022
pared added a commit to pared/dvc that referenced this pull request Jul 29, 2022
@pared
cmd: plots: update messaging
8dc79a1 
Related: iterative/dvc.org#3691
pared added a commit to iterative/dvc that referenced this pull request Aug 1, 2022
@pared
cmd: plots: update messaging
ed62ddb 
Related: iterative/dvc.org#3691
@jorgeorpinel jorgeorpinel mentioned this pull request Aug 1, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jorgeorpinel jorgeorpinel jorgeorpinel left review comments

@dberenbaum dberenbaum dberenbaum approved these changes

@shcheklein shcheklein shcheklein left review comments

@daavoo daavoo daavoo approved these changes

Assignees

@jorgeorpinel jorgeorpinel

@pared pared

Labels
A: docs Area: user documentation (gatsby-theme-iterative) C: ref Content of /doc/*-reference
Projects
No open projects
DVC
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@pared @dberenbaum @jorgeorpinel @shcheklein @daavoo @julieg18