Conversation
|
@jorgeorpinel and @shcheklein for some reason I don't see |
|
@dmpetrov yep, update sidebar.json - that should do the trick. |
Thank you @shcheklein ! Forgot about this :) |
jorgeorpinel
left a comment
jorgeorpinel
left a comment
There was a problem hiding this comment.
Small preemptive review of the dvc plot intro.
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. |
|
@jorgeorpinel and @pared pushed all the changes. Please take a look. |
jorgeorpinel
left a comment
jorgeorpinel
left a comment
There was a problem hiding this comment.
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
| ## 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. |
There was a problem hiding this comment.
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.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
- p.s. unresolving for myself to remember about "that can be a separate issue (to match dvc metrics with this ref)"
|
@jorgeorpinel 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 |
|
@jorgeorpinel wrong what is the status of this? |
I keep doing that...
Some review items are still pending. Are we taking it over yet? |
|
@jorgeorpinel I trust you on making this call :) don't have enough context on this. |
|
@shcheklein and @jorgeorpinel I just updated it:
Please take a look, |
| # plot diff | ||
|
|
||
| Show multiple versions of | ||
| [continuous metrics](/doc/command-reference/plot#continous-metrics) by plotting |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Agreed with @pared . Probably even plain explicit "non-scalar metric" would be better?
There was a problem hiding this comment.
yeah. it will be changed. all the terminology around continuous will be removed in the next iteration.
There was a problem hiding this comment.
- So let's leave "continuous" in this PR so we have a plot cmd ref for now, and update it in a following PR.
There was a problem hiding this comment.
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?
-
scalar metrics (now "scores") should be explained in the description of
dvc metricscmd ref index, and just mention the term in the plot/index Description with a link to there, without explaining (from #1186 (comment)) -
Suggestions about terms "array" and "series: #1186 (comment), #1186 (comment), and #1186 (comment)
These could be just committed now before merge if you agree with them.
-
Remove term "continuous" (from #1186 (review))
-
Explain plot templates! See treeverse/dvc#3577 (comment) and link to cmd ref from DVC output (core repo, see treeverse/dvc#3577 (review))
4 participants
Plot metrics.
Related to treeverse/dvc#3409