-
Notifications
You must be signed in to change notification settings - Fork 5
plots: standardise across DVC/Studio/VS Code
#142
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
Conversation
f815828 to
df9ef37
Compare
Codecov ReportAttention:
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. |
df9ef37 to
7ef62f5
Compare
38d7977 to
7f1a1c6
Compare
|
Need to consider flexible confusion matrix :( (see treeverse/vscode-dvc#2523) |
a5284c2 to
e42805b
Compare
5ac3bd6 to
8927210
Compare
| self._fill_color(split_anchors, optional_anchors) | ||
| self._fill_set_encoding(split_anchors, optional_anchors) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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.
dberenbaum
left a comment
There was a problem hiding this 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.
2fbdafa to
e9fa62f
Compare
aea5747 to
7054de6
Compare
dberenbaum
left a comment
There was a problem hiding this 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.
8fc0dd4 to
aea8187
Compare
Related to treeverse/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-renderwill now process and provide flexible plot encoding updates tovscode-dvcviadvc plots diff(e.g. stroke dash or shape). These updates will be provided under a newanchor_definitionskey in theplots diffoutput.dvc-renderwill also use the encoding updates to render theindex.htmlin 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 theconfusionandconfusion_normalizedtemplates. E.g.column: Used to insert filename or field into a flexible plot template to group plots and create new columns. Used in thebar_horizontalandbar_horizontal_sortedtemplates.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 thesmooth/linearfor the smooth signal.group_by_x/group_by_y: Similar togroup_bybut 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 thesmooth/lineartemplate for calculation of the values shown in the tooltip. E.g.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 thesmooth/linear/simpletemplates. E.g.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 thescatter/scatter_jittertemplates. E.g.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 notconfusion/confusion_normalized.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 treeverse/dvc#9931
Demos
index.html
Screen.Recording.2023-09-22.at.12.31.11.pm.mov
VS Code (treeverse/vscode-dvc#4734)
Screen.Recording.2023-11-27.at.12.14.35.pm.mov
Note:
vscode-dvcwill 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.