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

Plots #1186

Merged
merged 9 commits into from
May 7, 2020
Merged

Plots #1186

merged 9 commits into from
May 7, 2020

Conversation

dmpetrov
Copy link
Member

Plot metrics.

Related to iterative/dvc#3409

@shcheklein shcheklein temporarily deployed to dvc-landing-plots-wjaustbsuxuv April 26, 2020 11:47 Inactive
@shcheklein shcheklein temporarily deployed to dvc-landing-plots-wjaustbsuxuv April 26, 2020 15:50 Inactive
@dmpetrov
Copy link
Member Author

@jorgeorpinel and @shcheklein for some reason I don't see dvc plot & dvc plot show\diff in the menu. It looks like I missed something.
Could you please help me?

@shcheklein
Copy link
Member

@dmpetrov yep, update sidebar.json - that should do the trick.

@shcheklein shcheklein temporarily deployed to dvc-landing-plots-wjaustbsuxuv April 26, 2020 16:20 Inactive
@dmpetrov
Copy link
Member Author

sidebar

Thank you @shcheklein ! Forgot about this :)

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.

Small preemptive review of the dvc plot intro.

Please let us know when this is ready for full review.

content/docs/command-reference/plot/index.md Outdated Show resolved Hide resolved
content/docs/command-reference/plot/index.md Outdated Show resolved Hide resolved
content/docs/command-reference/plot/index.md Outdated Show resolved Hide resolved
content/docs/command-reference/plot/index.md Outdated Show resolved Hide resolved
content/docs/command-reference/plot/index.md Show resolved Hide resolved
@dmpetrov
Copy link
Member Author

Please let us know when this is ready for full review.

@jorgeorpinel thank you for the small changes.

I'm waiting for a bugfix while I can add one more part about the plot templates. It would be great to make review now. I'll incorporate the template later right here or as a separate PR.

@dmpetrov dmpetrov changed the title [WIP] Plots Plots Apr 27, 2020
Copy link
Contributor

@pared pared left a comment

Choose a reason for hiding this comment

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

Left few comments

content/docs/command-reference/plot/diff.md Outdated Show resolved Hide resolved
content/docs/command-reference/plot/diff.md Show resolved Hide resolved
content/docs/command-reference/plot/diff.md Show resolved Hide resolved
content/docs/command-reference/plot/diff.md Outdated Show resolved Hide resolved
content/docs/command-reference/plot/index.md Outdated Show resolved Hide resolved
content/docs/command-reference/plot/index.md Outdated Show resolved Hide resolved
content/docs/command-reference/plot/show.md Outdated Show resolved Hide resolved
@shcheklein shcheklein temporarily deployed to dvc-landing-plots-wjaustbsuxuv April 28, 2020 01:44 Inactive
@dmpetrov
Copy link
Member Author

@jorgeorpinel and @pared pushed all the changes. Please take a look.

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.

OK, another round of review on the basics of each document.

I didn't go into some details like Options and Examples but I can if you want.

We can also take this over when needed. Please lmk

content/docs/command-reference/plot/index.md Outdated Show resolved Hide resolved
content/docs/command-reference/plot/index.md Outdated Show resolved Hide resolved
Comment on lines 20 to 24
## Description

DVC provides a set of commands to visualize continuous metrics of machine
learning experiments in. Usual examples of plots are AUC curves, loss functions,
and confusion matrices.
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should just jump into explaining what continuous metrics are right here on the top of the description, and remove the Continuous metrics H3 header. Start with the

In contrast to [scalar metrics](/doc/command-reference/metrics), continous metrics represents a plot and ...

paragraph that's much further down rn. Idk if we need to explain scalar metrics here, probably not?

And just link to /doc/command-reference/plot directly from places where [continuous metrics] are mentioned.

This comment was marked as resolved.

Copy link
Member Author

Choose a reason for hiding this comment

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

All points are great! I rearranged this part. Please take a look.

It is still important to keep the scalars vs continuous difference somewhere. I keep it as a next section but rearranged from the plot point of view.

Copy link
Contributor

@jorgeorpinel jorgeorpinel Apr 29, 2020

Choose a reason for hiding this comment

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

It is still important to keep the scalars vs continuous difference somewhere

I agree! But I think the scalar metrics should be explained in the description of dvc metrics cmd ref index, and just mention the word here with a link to there, without explaining. But OK I guess that can be a separate issue (to match dvc metrics with this ref)

Copy link
Contributor

@jorgeorpinel jorgeorpinel Apr 29, 2020

Choose a reason for hiding this comment

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

  • p.s. unresolving for myself to remember about "that can be a separate issue (to match dvc metrics with this ref)"

content/docs/command-reference/plot/index.md Outdated Show resolved Hide resolved
content/docs/command-reference/plot/index.md Outdated Show resolved Hide resolved
content/docs/command-reference/plot/show.md Outdated Show resolved Hide resolved
content/docs/command-reference/plot/show.md Outdated Show resolved Hide resolved
content/docs/command-reference/plot/diff.md Outdated Show resolved Hide resolved
content/docs/command-reference/plot/show.md Outdated Show resolved Hide resolved
content/docs/command-reference/plot/index.md Outdated Show resolved Hide resolved
@jorgeorpinel jorgeorpinel temporarily deployed to dvc-landing-plots-wjaustbsuxuv April 28, 2020 05:01 Inactive
@shcheklein shcheklein temporarily deployed to dvc-landing-plots-wjaustbsuxuv April 29, 2020 09:33 Inactive
@dmpetrov
Copy link
Member Author

@jorgeorpinel thank you for the review. Please take a look at the changes.

@jorgeorpinel
Copy link
Contributor

thank you for the review. Please take a look at the changes

No problem. I resolved all the parts of the review that are now addressed or no longer relevant, and added one more comment. Still haven't reviewed everything in detail (options, examples) is it a good time to do so? And again, I can take this over at some point if needed, lmk guys cc @ivan

@shcheklein
Copy link
Member

@jorgeorpinel wrong @ ivan I think :)

what is the status of this?

@jorgeorpinel
Copy link
Contributor

jorgeorpinel commented May 5, 2020

wrong @ ivan

I keep doing that...

what is the status of this?

Some review items are still pending. Are we taking it over yet?

@shcheklein
Copy link
Member

@jorgeorpinel I trust you on making this call :) don't have enough context on this.

@dmpetrov
Copy link
Member Author

dmpetrov commented May 5, 2020

@shcheklein and @jorgeorpinel I just updated it:

  1. made all the asked changes
  2. added new plot options
  3. small API fixes (output file name)
  4. added custom plots section

Please take a look,

# plot diff

Show multiple versions of
[continuous metrics](/doc/command-reference/plot#continous-metrics) by plotting
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder whether we want to name them continuous. This word applies to functions. What about, for example, confusion matrix? Data for that type of plot is not continuous.

Copy link
Member

Choose a reason for hiding this comment

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

Agreed with @pared . Probably even plain explicit "non-scalar metric" would be better?

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah. it will be changed. all the terminology around continuous will be removed in the next iteration.

Copy link
Contributor

Choose a reason for hiding this comment

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

  • So let's leave "continuous" in this PR so we have a plot cmd ref for now, and update it in a following PR.

content/docs/command-reference/plot/index.md Outdated Show resolved Hide resolved
content/docs/command-reference/plot/show.md Outdated 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.

Some items pending but this whole doc needs another pass anyway so approving. Let's merge and address the stuff pending (below) along with the next PR?


@dmpetrov dmpetrov merged commit 492f002 into master May 7, 2020
@dmpetrov dmpetrov deleted the plots branch May 7, 2020 17:02
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.

4 participants