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

guide: make plots guide #3830

Merged
merged 13 commits into from
Aug 10, 2022
Merged

guide: make plots guide #3830

merged 13 commits into from
Aug 10, 2022

Conversation

dberenbaum
Copy link
Contributor

@dberenbaum dberenbaum commented Jul 28, 2022

This is a quick iteration to address #3818. I made a new guide page since I thought shoving that all into the dvcyaml/project structure would be way too much/messy and hard to find.

Also related: #2572

In review app: https://dvc-org-guide-plots-u9q3eipvdr.herokuapp.com/doc/user-guide/plots

@dberenbaum dberenbaum requested review from jorgeorpinel and pared July 28, 2022 19:43
@shcheklein shcheklein temporarily deployed to dvc-org-guide-plots-u9q3eipvdr July 28, 2022 19:44 Inactive
@github-actions
Copy link
Contributor

github-actions bot commented Jul 28, 2022

Link Check Report

All 59 links passed!

@shcheklein shcheklein temporarily deployed to dvc-org-guide-plots-u9q3eipvdr July 31, 2022 06:44 Inactive
@jorgeorpinel jorgeorpinel added A: docs Area: user documentation (gatsby-theme-iterative) C: guide Content of /doc/user-guide C: ref Content of /doc/*-reference labels Jul 31, 2022
Comment on lines 158 to 159
"label": "Plots Configuration and Visualization",
"slug": "plots"
Copy link
Contributor

@jorgeorpinel jorgeorpinel Jul 31, 2022

Choose a reason for hiding this comment

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

Name's a bit long. It'd be best if it fits into one line in the nav.

image

Also that the slug marches the title if possible (better for SEO AFAIK).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed. Do you think Plots is enough? What do you suggest?

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe "Data Visualization" for now?

Or something else that will let us include params and metrics (in general) in the future too, per #2572 but we can change it later.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe "Data Visualization" for now?

I like to at least use the word "Plots" or it feels like needless obfuscation. Do you think so, or is "Data Visualization" more accurate in some way?

Or something else that will let us include params and metrics (in general) in the future too, per #2572 but we can change it later.

I feel plots may need its own guide since it's a fairly complex subsystem of DVC on its own, but yeah, I guess it's premature to decide either way right now.

Copy link
Contributor

@jorgeorpinel jorgeorpinel Aug 3, 2022

Choose a reason for hiding this comment

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

Maybe Visualizing Plots for now?

Suggested change
"label": "Plots Configuration and Visualization",
"slug": "plots"
"slug": "visualizing-plots"

Up to you, I just think plain "Plots" is too generic. But we expect guide traffic to come from links in other docs (rather than by browsing the nav) so the name may not be that important.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Visualizing plots works and fits the style of the rest of the other guides, so I'll go with that.

This is mostly a style choice, but I prefer to keep it simple because IMHO it's easier to navigate if the sidebar looks like:

  • Experiments
  • Pipelines
  • Plots

It takes longer for me to find the right section when we add in these descriptive words like:

  • Experiments management
  • Data pipelines
  • Visualizing plots

Copy link
Contributor

Choose a reason for hiding this comment

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

Goes back to how exactly we want to organize the user guide. I don't think we can maintain detailed guides for every single feature so specific titles may be more appropriate. Up to you

Comment on lines 309 to 313
## Example: Offline HTML Template

The plots generated by `dvc plots` uses Vega-Lite JavaScript libraries, and by
default these load [online resources](https://vega.github.io/vega/usage/#embed).
There may be times when you need to produce plots without Internet access, or
Copy link
Contributor

Choose a reason for hiding this comment

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

... In the user guide we don't usually have separate examples like these. We embed them into the explanations instead. Think book format.

Copy link
Contributor

@jorgeorpinel jorgeorpinel Aug 3, 2022

Choose a reason for hiding this comment

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

all the plot template info is in the new guide now

... ideally we would dissolve the examples into the explanations themselves then, as suggested here.

I'm trying to reorganize first as a quick iteration

OK. Let's try to have a check box somewhere to follow up on this.

Comment on lines 76 to 79
## Defining plots

In order to create visualizations, users need to provide the data and
(optionally) configuration that will help customize the plot. DVC provides two
Copy link
Contributor

@jorgeorpinel jorgeorpinel Jul 31, 2022

Choose a reason for hiding this comment

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

... what I'm suggesting to summarize (heavily probably) and move everything in a good chunk of this section to the dvc.yaml spec, since it's mainly about the schema.

Copy link
Contributor

Choose a reason for hiding this comment

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

2... use the guide to show how to setup plots and leave the cmd ref and dvc.yaml pages for lookup purposes

We agree on this @dberenbaum. I just think the Top-level plots explanation can be shorter and leave most schema details for the dvc.yaml "guide".

E.g. phrases like "should be defined ... under plots key" or "Configuration ... should be a dictionary", and the whole Available configuration fields section (as noted). In short, this section is currently a more complete ref than the actual ref.

Note that in the case of CSV/TSV metrics files, column names from the table
header (first row) are equivalent to field names.

### Custom templates
Copy link
Contributor

Choose a reason for hiding this comment

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

Unrelated but should we rename this Custom Vega templates since there's a Custom HTML templates H2 right after this?

@shcheklein shcheklein temporarily deployed to dvc-org-guide-plots-u9q3eipvdr July 31, 2022 07:41 Inactive
@shcheklein
Copy link
Member

Only thing to add to this. I would try to avoid making command references way too short. They should be like a man page still. It has some dry formal description of a behavior, some examples to grasp the idea fast, etc.

User guide is definitely higher level. In case of plots, similar to the current index page https://dvc.org/doc/command-reference/plots I think. And can go even a bit higher level to describe things like how to get the data in the first place - dvclive, dump it with Python. Can describe the differences between this and usual loggers, etc, etc.

@jorgeorpinel
Copy link
Contributor

jorgeorpinel commented Aug 4, 2022

avoid making command references way too short. They should be like a man page

Agree

User guide is definitely higher level

Hmmm. Yeah TBH the UG is the most confusing part of our docs conceptually rn, probably because it is both high-level and the most low-level some times (e.g. it includes implementation details). Maybe there's no way to completely separate them if we want a true, book-like "guide". But also the level of effort to keep a complete UG like that (which may overlap a lot with other types of content) is pretty high -- something to consider.

p.s. other product docs have very few and strategic guides (in that sense), mainly due to lack of capacity I think.

@dberenbaum
Copy link
Contributor Author

User guide is definitely higher level. In case of plots, similar to the current index page https://dvc.org/doc/command-reference/plots I think.

To clarify, that's what this PR currently does.

However, I understand the concern from @jorgeorpinel that what's there is too low level and more of a reference than a guide. 🤔 ⌛

`test_logs.csv` against the `epoch`. Note that both files have to have `epoch`
field.

### Available configuration fields
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@pared Should template be added here?

@shcheklein shcheklein temporarily deployed to dvc-org-guide-plots-u9q3eipvdr August 5, 2022 21:11 Inactive
@dberenbaum
Copy link
Contributor Author

To clarify, that's what this PR currently does.

However, I understand the concern from @jorgeorpinel that what's there is too low level and more of a reference than a guide. 🤔 ⌛

I did another pass and made some changes:

  • Moved a lot of the top-level plots configuration and available configuration fields to the dvcyaml guide.
  • Moved custom template configuration to the plots templates cmd ref.
  • Moved most examples to the plots show cmd ref.
  • Duplicated a few high-level examples in the plots user guide.

* guide: add high-level plots guide content

* guide: fix plots guide links

* Restyled by prettier (#3870)

Co-authored-by: Restyled.io <[email protected]>

Co-authored-by: restyled-io[bot] <32688539+restyled-io[bot]@users.noreply.github.com>
Co-authored-by: Restyled.io <[email protected]>
@shcheklein shcheklein temporarily deployed to dvc-org-guide-plots-u9q3eipvdr August 10, 2022 14:39 Inactive
@shcheklein shcheklein temporarily deployed to dvc-org-guide-plots-u9q3eipvdr August 10, 2022 14:43 Inactive
@dberenbaum dberenbaum merged commit 2361373 into main Aug 10, 2022
@dberenbaum dberenbaum deleted the guide-plots branch August 10, 2022 15:01
Comment on lines +157 to +159
{
"slug": "visualizing-plots"
},
Copy link
Contributor

@jorgeorpinel jorgeorpinel Aug 22, 2022

Choose a reason for hiding this comment

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

This can be shortened to just

Suggested change
{
"slug": "visualizing-plots"
},
"visualizing-plots"

p.s. there's an unnecessary \n in line 167.

Copy link
Contributor

Choose a reason for hiding this comment

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

UPDATE: I'm fixing these in #3903

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jorgeorpinel Please don't! I already submitted a PR in #3902

Copy link
Contributor

Choose a reason for hiding this comment

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

I already submitted a PR in #3902

Oops. My bad

@jorgeorpinel
Copy link
Contributor

jorgeorpinel commented Aug 22, 2022

Thanks for all the updates and sorry for late check @dberenbaum . Left some specific suggestions (minor) above...

https://dvc.org/doc/user-guide/visualizing-plots and https://dvc.org/doc/user-guide/project-structure/dvcyaml-files#top-level-plot-definitions did end up having all the info they need I think. Of course we can always improve on docs but at least they seem to serve their intended purpose as they are now 👍🏼

Some thoughts:

  • "Save data to a supported file format": Should we emphasize that the data is produced by a data pipeline? Link to #generating-plots-files maybe.
  • "Show all plots in a single view": Can the figures be consolidated into a single smaller image? Lots of scrolling rn.
  • We can combine the "Generating plots files" and "Supported file formats" sections, I think. Maybe "Plot data sources" or some other title.
  • Move "Top-level plot" section before "Stage plots" ?
    And link to https://dvc.org/doc/user-guide/project-structure/dvcyaml-files#top-level-plot-definitions directly.
  • and...

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.

...Relatively minor review of dvc.yaml doc changes (for readability)

Comment on lines +478 to +482
The list of `plots` contains one or more user-defined top-level plots (paths
relative to the location of `dvc.yaml`).

This example makes output `auc.json` viable for visualization, configuring keys
`fpr` and `tpr` as X and Y axis, respectively:
Every plot has to have its own ID. Configuration, if provided, should be a
dictionary.
Copy link
Contributor

Choose a reason for hiding this comment

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

Combine intro P

The list of `plots` contains one or more user-defined top-level plots (paths
relative to the location of `dvc.yaml`).
Every plot needs a unique file path as identifier.
Optional configuration can be given as a dictionary.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Every plot needs a unique file path as identifier.

You can have many different plots all based on the same file.

Copy link
Contributor

Choose a reason for hiding this comment

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

different plots all based on the same file

I see. This "path vs ID" situation is a bit confusing. Explanations could improve on that I think... ⌛

jorgeorpinel added a commit that referenced this pull request Aug 30, 2022
dberenbaum pushed a commit that referenced this pull request Sep 7, 2022
* guide: edits
per #3830 (review)

* guide: improvements to Plots

* guide: improve Plot spec

* ref: improve plots a little and
other edits in Plots guides

* Update content/docs/user-guide/visualizing-plots.md

* guide: list top-level plots before stage

* guide: combine jorge and dave edits for plots

* guide: consolidate plots guide

Co-authored-by: dberenbaum <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A: docs Area: user documentation (gatsby-theme-iterative) C: guide Content of /doc/user-guide C: ref Content of /doc/*-reference
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants