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: standardise across DVC/Studio/VS Code #142

Merged
merged 40 commits into from
Dec 5, 2023
Merged

Conversation

mattseddon
Copy link
Member

@mattseddon mattseddon commented Sep 13, 2023

Related to iterative/dvc#9940.

This PR updates the Vega templates with new anchors that will help us standardise the products' behaviour and appearance.

Approach

Extra optional anchors have been added to the default templates. These anchors will be used by both Studio and the VS Code extension to make encoding updates in the appropriate places. If the appropriate anchors are not in a provided template (e.g. a custom template) then we will no longer make encoding updates.

dvc-render will now process and provide flexible plot encoding updates to vscode-dvc via dvc plots diff (e.g. stroke dash or shape). These updates will be provided under a new anchor_definitions key in the plots diff output. dvc-render will also use the encoding updates to render the index.html in a way that will match the other products.

New Optional Anchors

Out of all of the new anchors the only ones that I foresee users needing to know/care about are detailed in this PR (i.e. <DVC_METRIC_COLOR> | <DVC_METRIC_PLOT_HEIGHT> | <DVC_METRIC_PLOT_WIDTH>). The other values can be hardcoded if the appropriate data groupings are known before we try to render the plot. I.e. users have the benefit of not having to create dynamic templates.

row: Used to insert filename or field into a flexible plot template to group plots and create new rows. Used in the confusion and confusion_normalized templates. E.g.

image

column: Used to insert filename or field into a flexible plot template to group plots and create new columns. Used in the bar_horizontal and bar_horizontal_sorted templates.

group_by: Used to group data to the appropriate level for a flexible plot template, entry is an array of field names. Will be rev + any combination of filename and field. Used in the smooth/linearfor the smooth signal.

group_by_x / group_by_y: Similar to group_by but with the x/y field added.

pivot_field: Used to group data into a single field at the appropriate level. This field is then used by the pivot calculation. Will be rev + any combination of filename and field. Currently only used by the smooth/linear template for calculation of the values shown in the tooltip. E.g.

image

stroke_dash: Used to insert filename or field into a flexible plot template by creating groups of stroke dashes for distinct combinations of filename fields. Currently used in the smooth/linear/simple templates. E.g.

image

shape: Used to insert filename or field into a flexible plot template by creating groups of shapes for distinct combinations of filename fields. Currently used in the scatter/scatter_jitter templates. E.g.

image

tooltip: Used to dynamically add data into a flexible plot template tooltip. Only the x/y/rev values and varied fields will be added to the tooltip.

color: Used to group rev information across separate plots. Used by all of the templates that are not confusion/confusion_normalized.

image

plot_width: Used by the VS Code extension/Studio to dynamically resize the width of plots. Set to 300 in the HTML output.

plot_height: Used by the VS Code extension/Studio to dynamically resize the height of plots. Set to 300 in the HTML output.

The associated changes for DVC are in iterative/dvc#9931

Demos

index.html

Screen.Recording.2023-09-22.at.12.31.11.pm.mov

VS Code (iterative/vscode-dvc#4734)

Screen.Recording.2023-11-27.at.12.14.35.pm.mov

Note: vscode-dvc will continue to provide its own color encoding (match rev to a color) for now. I have left the door open for a "rev to color" mapping to be provided to the CLI in future. The code to calculate flexible plot encoding updates will be dropped from the VS Code extension.

Studio (https://github.com/iterative/studio/pull/8242 / https://github.com/iterative/studio/pull/8301 / https://github.com/iterative/studio/pull/8340)

Screen.Recording.2023-11-23.at.1.18.51.pm.mov

Note: Studio has to calculate all of the required anchor definitions.

@mattseddon mattseddon force-pushed the update-templates branch 2 times, most recently from f815828 to df9ef37 Compare September 13, 2023 10:10
@codecov-commenter
Copy link

codecov-commenter commented Sep 13, 2023

Codecov Report

Attention: 2 lines in your changes are missing coverage. Please review.

Comparison is base (5280aa2) 96.12% compared to head (aea8187) 97.49%.

Files Patch % Lines
src/dvc_render/vega.py 98.59% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #142      +/-   ##
==========================================
+ Coverage   96.12%   97.49%   +1.36%     
==========================================
  Files          19       19              
  Lines         825     1036     +211     
  Branches      132      172      +40     
==========================================
+ Hits          793     1010     +217     
+ Misses         17       15       -2     
+ Partials       15       11       -4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

src/dvc_render/vega.py Outdated Show resolved Hide resolved
src/dvc_render/vega.py Outdated Show resolved Hide resolved
@mattseddon
Copy link
Member Author

Need to consider flexible confusion matrix :( (see iterative/vscode-dvc#2523)

src/dvc_render/vega.py Outdated Show resolved Hide resolved
@mattseddon mattseddon force-pushed the update-templates branch 3 times, most recently from a5284c2 to e42805b Compare October 5, 2023 04:55
@mattseddon mattseddon force-pushed the update-templates branch 3 times, most recently from 5ac3bd6 to 8927210 Compare October 12, 2023 02:33
tests/test_vega.py Outdated Show resolved Hide resolved
tests/test_vega.py Outdated Show resolved Hide resolved
tests/test_vega.py Outdated Show resolved Hide resolved
tests/test_vega.py Outdated Show resolved Hide resolved
tests/test_vega.py Outdated Show resolved Hide resolved
tests/test_vega.py Outdated Show resolved Hide resolved
tests/test_vega.py Outdated Show resolved Hide resolved
tests/test_vega.py Outdated Show resolved Hide resolved
Comment on lines +230 to +231
self._fill_color(split_anchors, optional_anchors)
self._fill_set_encoding(split_anchors, optional_anchors)
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor: This is probably outside the scope of this PR and maybe personal bias, but some of why I find the plots code so hard to read is because everything is defined as a class attribute and mutated in place rather than being written in a more functional style. I have no idea what these methods are actually modifying.

Copy link
Member Author

@mattseddon mattseddon Dec 3, 2023

Choose a reason for hiding this comment

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

I can rewrite it as functions and reduce the mutation of class properties if you think that would help.

Currently, the only properties on the class are _split_content (which gets spat out as anchor_definitions and template which is either the filled or partially filled in Vega template.

Copy link
Member Author

Choose a reason for hiding this comment

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

(I was trying to fit in with the established code)

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, it's outside the scope of this PR, I just wanted to make a note of it. Let's not expand the scope here.

Copy link
Contributor

@dberenbaum dberenbaum left a comment

Choose a reason for hiding this comment

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

Thanks @mattseddon! It's not the easiest code to read, but I don't see any major issues. I do have a few minor comments and a few tests where I am either confused or think we could add some more cases.

Copy link
Contributor

@dberenbaum dberenbaum left a comment

Choose a reason for hiding this comment

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

@mattseddon Thanks for the quick updates! I just want to clarify whether to add rev cases to the tests or to keep them only in scatter. Otherwise LGTM.

@mattseddon mattseddon merged commit f3c3f66 into main Dec 5, 2023
13 checks passed
@mattseddon mattseddon deleted the update-templates branch December 5, 2023 04:05
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.

5 participants