Skip to content

Get Started: explaining code.zip contents, and plots axis options#1514

Merged
shcheklein merged 6 commits into
masterfrom
xnutsive/get-started-updates
Jul 5, 2020
Merged

Get Started: explaining code.zip contents, and plots axis options#1514
shcheklein merged 6 commits into
masterfrom
xnutsive/get-started-updates

Conversation

@natikgadzhi
Copy link
Copy Markdown
Contributor

@natikgadzhi natikgadzhi commented Jul 1, 2020

  1. Added the more explicit ls -R to show that the code.zip already has params.yaml.
  2. Added dvc plots modify details to experiments guide.

❗ Please read the guidelines in the Contributing to the Documentation list if you make any substantial changes to the documentation or JS engine.

🐛 Please make sure to mention Fix #issue (if applicable) in the description of the PR. This causes GitHub to close it automatically when the PR is merged.

Please choose to allow us to edit your branch when creating the PR.

Thank you for the contribution - we'll try to review it as soon as possible. 🙏

Comment thread content/docs/start/data-pipelines.md Outdated
Comment thread content/docs/start/experiments.md Outdated
Comment thread content/docs/start/experiments.md
Comment thread content/docs/start/experiments.md Outdated
@natikgadzhi
Copy link
Copy Markdown
Contributor Author

natikgadzhi commented Jul 1, 2020 via email

@jorgeorpinel
Copy link
Copy Markdown
Contributor

Sure @xnutsive take your time.

Qq: how do I trigger the review app myself

Can't when the PR comes from a fork. We have to do it for you. But you can easily run the app locally! See https://dvc.org/doc/user-guide/contributing/docs

@natikgadzhi natikgadzhi changed the title > You may disregard these recommendations if you used the **Edit on GitHub** button from dvc.org to improve a doc in place. Get Started: explaining code.zip contents, and plots axis options Jul 2, 2020
@natikgadzhi natikgadzhi force-pushed the xnutsive/get-started-updates branch from 2cf3466 to 6bf7b4d Compare July 2, 2020 05:08
@shcheklein shcheklein temporarily deployed to dvc-landing-xnutsive-ge-1visdu July 2, 2020 05:08 Inactive
@natikgadzhi natikgadzhi marked this pull request as ready for review July 2, 2020 05:08
@shcheklein shcheklein temporarily deployed to dvc-landing-xnutsive-ge-1visdu July 2, 2020 05:14 Inactive
Comment thread content/docs/start/data-pipelines.md Outdated
@jorgeorpinel jorgeorpinel temporarily deployed to dvc-landing-xnutsive-ge-1visdu July 2, 2020 17:07 Inactive
Comment thread content/docs/start/data-pipelines.md Outdated
Comment thread content/docs/start/experiments.md Outdated
Comment on lines +225 to +227
If you report more than one metric to the `plots` file, you can provide
metric names to `dvc plots diff` command with `-x` and `-y` options.
See more here: [`dvc plots diff` options](https://dvc.org/doc/command-reference/plots/diff#options).
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Probably not needed. WDYT @shcheklein ?

If keeping this, I'd put it in a block quote (>) and start "The -x and -y options select which data series..." or something like that but it's going to be difficult to write it in a way that doesn't require further explanations (for example what is a data series?) so again, probably best to avoid this...

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Maybe just "> See dvc plots diff for more info on its options, such as -x and -y used above."

Copy link
Copy Markdown
Contributor

@jorgeorpinel jorgeorpinel Jul 2, 2020

Choose a reason for hiding this comment

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

Notice that there's no need to make manual links from DVC commands. they get auto-linked when put in back ticks `.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The goal of this paragraph, when I wrote this, was to:

  1. Explain how to plot different metrics if needed, and explain that that's actually possible, spark interest to experiment with different metrics like that.
  2. Give an example of how to do this (actually, the example plot above does this in an implicit way).

You guys probably have a better judgement on whether it's a good idea to explain this in the get started series.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think we want to keep the GS as simple and focused as possible. Users with interest on this can click on the dvc plots link that already exists above and learn everything about metrics and plots. We also don't talk about dvc plots show, for example... No way to cover all these options in GS directly. I personally vote to remove this note altogether but I'm OK with a very short one like I suggested above, since this is the very bottom of the doc anyway.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Yep, I would omit it as well. In other places if explain options we explain them in the <details> section. Otherwise PR looks great to me!

@jorgeorpinel
Copy link
Copy Markdown
Contributor

jorgeorpinel commented Jul 2, 2020

@xnutsive the Ci checks are failing. I think it's the formatting since Restyled is also failing. Please follow the formatting explained in the contribution guide 🙂 or we can merge the Restyled PR copy instead.

@natikgadzhi
Copy link
Copy Markdown
Contributor Author

@jorgeorpinel, that was my bad — hooks didn't run because I removed yarn, and I didn't pay attention. Fixed both the formatting, and the content. Thanks for being patient. ;)

@shcheklein shcheklein temporarily deployed to dvc-landing-xnutsive-ge-1visdu July 3, 2020 03:25 Inactive
@natikgadzhi natikgadzhi requested a review from jorgeorpinel July 3, 2020 03:26
Copy link
Copy Markdown
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.

Np. no rush here.

Approved!

@shcheklein shcheklein merged commit ee515b8 into master Jul 5, 2020
@shcheklein
Copy link
Copy Markdown
Contributor

thanks @xnutsive !

@jorgeorpinel jorgeorpinel deleted the xnutsive/get-started-updates branch July 6, 2020 18:51
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.

3 participants