-
Notifications
You must be signed in to change notification settings - Fork 5
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
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
26d0d21
to
925d4e8
Compare
38d7977
to
7f1a1c6
Compare
Need to consider flexible confusion matrix :( (see iterative/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.
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
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 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 tovscode-dvc
viadvc plots diff
(e.g. stroke dash or shape). These updates will be provided under a newanchor_definitions
key in theplots diff
output.dvc-render
will also use the encoding updates to render theindex.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 theconfusion
andconfusion_normalized
templates. E.g.column
: Used to insert filename or field into a flexible plot template to group plots and create new columns. Used in thebar_horizontal
andbar_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 thesmooth
/linear
for the smooth signal.group_by_x
/group_by_y
: Similar togroup_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 thesmooth
/linear
template 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
/simple
templates. 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_jitter
templates. 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 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.