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

Initial support for flexible plots #7477

Merged
merged 1 commit into from Jul 1, 2022
Merged

Initial support for flexible plots #7477

merged 1 commit into from Jul 1, 2022

Conversation

pared
Copy link
Contributor

@pared pared commented Mar 18, 2022 •
edited

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

@pared pared force-pushed the 7086_flexible_plots_initial branch 2 times, most recently from 5cbe84f to 0ea16fc Compare March 25, 2022 17:57
@pared
Copy link
Contributor Author

pared commented Apr 7, 2022

This change might end up the ordering of plots handled in #5009. Writing it down for future reference

@pared pared force-pushed the 7086_flexible_plots_initial branch from fb228ed to c466bb6 Compare April 7, 2022 11:09
@pared pared force-pushed the 7086_flexible_plots_initial branch from 18a12e3 to 4d50d5a Compare April 20, 2022 20:17
@pared pared added enhancement Enhances DVC feature is a feature A: plots Related to the plots labels Apr 26, 2022
@pared pared force-pushed the 7086_flexible_plots_initial branch 6 times, most recently from ad1e6aa to 44bd33d Compare May 2, 2022 14:41
@pared pared force-pushed the 7086_flexible_plots_initial branch 3 times, most recently from 5283563 to e95edc7 Compare May 9, 2022 16:39
@daavoo daavoo mentioned this pull request May 9, 2022
Closed
daavoo
daavoo reviewed May 9, 2022
View reviewed changes
dvc/render/vega_converter.py Outdated Show resolved Hide resolved
dberenbaum
dberenbaum reviewed May 12, 2022
View reviewed changes
dvc/render/match.py
plot_datapoints.extend(dps)
renderer_id = plot_id
if "title" not in final_props:
final_props["title"] = renderer_id
Copy link
Contributor

Choose a reason for hiding this comment

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

Similar to stripping redundant info from the legend, it would be nice to strip dvc.yaml:: or other path info from plots IDs in the title unless there is a conflict. Alternatively, that info could be shown in a subtitle or some other way to make it less distracting.

Copy link
Contributor

Choose a reason for hiding this comment

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

When an explicit title: is set, should we also include the path somewhere, at least when they is a conflict? Right now you can have plots in two different directories with title: Accuracy and there is no way to distinguish them in the html.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, ill look into that

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, so in this case, we could relatively simple include that by modyfing dvc-render. (It would be backward compatible). I propose to do it after the release.

@dberenbaum

This comment was marked as outdated.

Sign in to view
dberenbaum
dberenbaum reviewed May 12, 2022
View reviewed changes
dvc/commands/plots.py Outdated Show resolved Hide resolved
@dberenbaum
Copy link
Contributor

Should targets include any plot key so that it's possible to do dvc plots show acc_train_vs_test?

@pared pared force-pushed the 7086_flexible_plots_initial branch 2 times, most recently from 189fd84 to f14ec12 Compare May 13, 2022 10:37
@pared

This comment was marked as outdated.

Sign in to view
@pared
Copy link
Contributor Author

pared commented May 13, 2022 •
edited

Should targets include any plot key so that it's possible to do dvc plots show acc_train_vs_test?

As of today we can address the plots by:
path - should work for old type plots (plots output)
plot_id - will match any {plot_id} within any config file, eg. plots show plot_id will target both dvc.yaml::plot_id and subdir/dvc.yaml::plot_id (assuming they exist)
config_path::plot_id - should match only particular plot_id within config_path

regarding non-dvc configs (--from-config) if config_path is not stage file, from-config still needs to be provided

@dberenbaum
Copy link
Contributor

One more thing to do: update the help text for targets in show and diff. They both seem to work as expected when given a plots name key as a target, so we should mention this in the command help.

@pared pared force-pushed the 7086_flexible_plots_initial branch from 2965d6c to ae54957 Compare May 17, 2022 11:16
daavoo
daavoo reviewed May 18, 2022
View reviewed changes
dvc/commands/plots.py Outdated Show resolved Hide resolved
@pared pared force-pushed the 7086_flexible_plots_initial branch from 2fbab97 to 498089a Compare June 27, 2022 18:04
pared added a commit to pared/dvc.org that referenced this pull request Jun 28, 2022
@pared
cmd-ref: plots: flexible plots docs
de12fcd 
Related: iterative/dvc#7477
Related: iterative#2956
pared added a commit to pared/dvc.org that referenced this pull request Jun 28, 2022
@pared
cmd-ref: plots: flexible plots docs
77bab38 
Related: iterative/dvc#7477
Related: iterative#2956
pared added a commit to pared/dvc.org that referenced this pull request Jun 28, 2022
@pared
cmd-ref: plots: flexible plots docs
7334445 
Related: iterative/dvc#7477
Related: iterative#2956
@pared pared mentioned this pull request Jun 28, 2022
Closed
@pared pared force-pushed the 7086_flexible_plots_initial branch 3 times, most recently from 63582e6 to 134a762 Compare June 30, 2022 12:41
@daavoo daavoo mentioned this pull request Jun 30, 2022
Open
@pared pared force-pushed the 7086_flexible_plots_initial branch from 134a762 to ad4ef9f Compare July 1, 2022 08:50
pared added a commit to pared/dvc.org that referenced this pull request Jul 1, 2022
@pared
cmd-ref: plots: flexible plots docs
ac87c80 
Related: iterative/dvc#7477
Related: iterative#2956
@pared pared force-pushed the 7086_flexible_plots_initial branch from ad4ef9f to a834fb0 Compare July 1, 2022 09:14
pared added a commit to pared/dvc.org that referenced this pull request Jul 1, 2022
@pared
cmd-ref: plots: flexible plots docs
f9df026 
Related: iterative/dvc#7477
Related: iterative#2956
@pared pared merged commit fbb98a8 into iterative:main Jul 1, 2022
pared added a commit to pared/dvc.org that referenced this pull request Jul 4, 2022
@pared
cmd-ref: plots: flexible plots docs
86e0d7c 
Related: iterative/dvc#7477
Related: iterative#2956
pared added a commit to pared/dvc.org that referenced this pull request Jul 4, 2022
@pared
cmd-ref: plots: flexible plots docs
14a8a0a 
Related: iterative/dvc#7477
Related: iterative#2956
pared added a commit to pared/dvc.org that referenced this pull request Jul 6, 2022
@pared
cmd-ref: plots: flexible plots docs
0ff761a 
Related: iterative/dvc#7477
Related: iterative#2956
@daavoo daavoo mentioned this pull request Jul 11, 2022
Closed
@pared pared mentioned this pull request Jul 13, 2022
Merged
2 tasks
pared added a commit to pared/dvc.org that referenced this pull request Jul 18, 2022
@pared
cmd-ref: plots: flexible plots docs
b308ada 
Related: iterative/dvc#7477
Related: iterative#2956
pared added a commit to pared/dvc.org that referenced this pull request Jul 18, 2022
@pared
cmd-ref: plots: flexible plots docs
675cc0c 
Related: iterative/dvc#7477
Related: iterative#2956
pared added a commit to pared/dvc.org that referenced this pull request Jul 20, 2022
@pared
cmd-ref: plots: flexible plots docs
5372a2c 
Related: iterative/dvc#7477
Related: iterative#2956
pared added a commit to pared/dvc.org that referenced this pull request Jul 20, 2022
@pared
cmd-ref: plots: flexible plots docs
826c9ed 
Related: iterative/dvc#7477
Related: iterative#2956
pared added a commit to pared/dvc.org that referenced this pull request Jul 25, 2022
@pared
cmd-ref: plots: flexible plots docs
097d013 
Related: iterative/dvc#7477
Related: iterative#2956
pared added a commit to pared/dvc.org that referenced this pull request Jul 28, 2022
@pared
cmd-ref: plots: flexible plots docs
2e96596 
Related: iterative/dvc#7477
Related: iterative#2956
dberenbaum added a commit to iterative/dvc.org 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 dberenbaum mentioned this pull request Sep 20, 2022
Open
@dberenbaum dberenbaum mentioned this pull request Sep 26, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dberenbaum dberenbaum dberenbaum approved these changes

@daavoo daavoo daavoo approved these changes

Assignees
No one assigned
Labels
A: plots Related to the plots enhancement Enhances DVC feature is a feature
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

plots: flexible dvc.yaml spec
3 participants
@pared @dberenbaum @daavoo